Harden retrieval and forgetting runtime paths

[Atharva-Kanherkar]

Summary

• filter superseded semantic memories out of default recall while keeping internal admin paths able to include them
• report explicit contradiction check status on semantic writes and single-flight forgetting runs with an already_running response on overlap
• emit memory.updated for procedural outcome recording and wire the shared contradiction detector/runtime paths consistently

Why

Issue #59 hardens the core runtime behavior for the MCP boundary. Before this change, superseded facts could still leak into retrieval, contradiction failures were indistinguishable from empty results, forgetting cycles could overlap, and procedural outcome updates changed state without emitting an observable event.

Validation

• python -m py_compile api/app.py stores/semantic_store.py stores/procedural_store.py forgetting/contradiction.py forgetting/service.py runtime.py events/logger.py tests/test_api_runtime_edges.py tests/test_forgetting_integration.py tests/test_forgetting_service.py tests/test_logger.py tests/test_procedural_store.py tests/test_semantic_store.py
• live localhost smoke checks against the running FastAPI server for contradiction status responses, memory.updated event emission, and superseded semantic filtering after resolve

Closes #59

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agentic-memory	Ready	Preview, Comment	Mar 29, 2026 8:51pm

[Atharva-Kanherkar]

@claude Please review the whole pr with every decision tree this code has touched, previously and now. Please see every decision tree that affects the change from start to end. Please make a plan, see the code, make a decsison tree, see that code, review it, seee old code, review it, and trhen come to a fianl judgement.

[claude]

Claude finished @Atharva-Kanherkar's task in 3m 19s —— View job

---

PR Review: Harden retrieval and forgetting runtime paths

Plan:

• Read CLAUDE.md for repo guidelines
• Read all 13 changed files (current branch state)
• Get full before/after git diff for every changed file
• Trace every decision tree end-to-end
• Cross-reference tests with production code
• Final judgement

---

Decision Trees: Before → After

1. SemanticStore.retrieve_by_vector — Superseded filtering

Before:

retrieve_by_vector(vector, top_k=5)
  n_results = top_k                          ← exact count, no filtering
  query ChromaDB(n_results=top_k)
  for each result → append(record, similarity)
  return pairs

All records including superseded ones were returned. Callers had no way to opt-out of stale data.

After:

retrieve_by_vector(vector, top_k=5, include_superseded=False)
  if not include_superseded:
    n_results = max(1, collection.count())   ← fetch ENTIRE collection
  else:
    n_results = max(1, top_k)
  query ChromaDB(n_results=n_results)
  for each result:
    if not include_superseded and record.superseded_by: skip
    else: append; if len(pairs) >= top_k: break
  return pairs (≤ top_k active records)

The same logic mirrors through retrieve() and get_all_records() / _all_records().

Finding — Performance O(N) scan on every query: When include_superseded=False (the new default), n_results is set to max(1, collection.count()), fetching all records from ChromaDB on every retrieval call. This is intentional — it's the only way to guarantee top_k active results when an unknown fraction may be superseded — but it turns every similarity search into a full-collection scan. For a memory system with thousands of entries this could become noticeably slow. Worth documenting or guarding with a future note. Fix this →

---

2. ContradictionDetector.find_likely_duplicates_batch — Admin opt-in

Before:

for record in get_all_records(include_embeddings=True):
    ← NEW DEFAULT would now filter superseded ← BUG if not fixed
    matches = retrieve_by_vector(embedding, top_k=2)
    ← NEW DEFAULT would now filter superseded ← BUG if not fixed

After:

for record in get_all_records(include_embeddings=True, include_superseded=True):
    matches = retrieve_by_vector(embedding, top_k=2, include_superseded=True)

Both calls correctly opt into the full set. This fix is necessary — the duplicate detector must reason over all records to correctly flag and then prune superseded content. Well done.

find_potential_contradictions (new-record contradiction lookup) does not pass include_superseded=True:

raw_results = self._store.retrieve_by_vector(stored_record.embedding, top_k=max(1, top_k + 1))

This is correct: when checking contradictions for a new write, you only want to compare against active (non-superseded) records.

---

3. ForgettingService._scan_records — Consistency fix

Before:

for store in self._stores.values():
    store.get_all_records(include_embeddings=True)
    ← semantic store under new default would silently drop superseded records
    ← the whole point of the forgetting cycle is to prune them!

After:

for memory_type, store in self._stores.items():
    if memory_type == "semantic":
        store.get_all_records(include_embeddings=True, include_superseded=True)
    else:
        store.get_all_records(include_embeddings=True)

The forgetting cycle correctly opts into superseded records for semantic scanning. Without this fix, superseded semantic memories would silently vanish from the scan and never be pruned. This is the most critical correctness fix in the PR.

---

4. ForgettingService.__init__ — Constructor API change

Before:

def __init__(self, ..., event_bus: EventBus | None = None,
             contradiction_detector: ContradictionDetector | None = None):
    self._duplicate_detector = contradiction_detector or ContradictionDetector(semantic_store)

After:

def __init__(self, ..., contradiction_detector: ContradictionDetector,
             event_bus: EventBus | None = None):
    self._duplicate_detector = contradiction_detector

Two changes here:

1. contradiction_detector is now required (not | None) and has moved before event_bus (keyword-only ordering change).
2. The fallback ContradictionDetector(semantic_store) is removed — caller must always wire it explicitly.

This is a breaking signature change for any direct callers. All known callsites (runtime.py, tests) were updated. The removal of the internal fallback is a good call — it forces callers to be explicit and prevents a hidden second detector being created with no event bus.

Finding — _NoopDuplicateDetector in tests is correctly minimal:

class _NoopDuplicateDetector:
    def find_likely_duplicates_batch(self, threshold=0.95):
        return []

Used when duplicate_pairs=None. Correctly satisfies the required contract without the overhead of a real ContradictionDetector.

---

5. _safe_contradiction_lookup — Error classification

Before:

try:
    return [ContradictionCandidate, ...]
except ValueError:
    return []          ← silently empty
# non-ValueError: propagates → HTTP 500

After:

try:
    return ContradictionCheckResult(status="completed", candidates=[...])
except ValueError as exc:
    return ContradictionCheckResult(status="skipped", candidates=[], detail=str(exc))
except Exception as exc:
    return ContradictionCheckResult(status="error", candidates=[], detail=str(exc))

The ContradictionCheckResult dataclass is a clean addition — it surfaces what happened instead of callers guessing from an empty list.

Finding — broad except Exception swallows programming errors: The old code intentionally let non-ValueError exceptions (e.g., TypeError, AttributeError from a misconfigured detector) propagate as HTTP 500. The new code catches all of them and returns status: "error" with a 200 response. The test that previously asserted status_code == 500 was updated to assert status_code == 200. This is a deliberate design choice — the primary store write succeeded, so returning 200 with a degraded contradiction status is reasonable — but it means real bugs in the detector (e.g., a TypeError from a code error) will silently surface as "status": "error" in the JSON body rather than causing observable failures in monitoring/alerting. Consider this →

---

6. run_forgetting_cycle — Single-flight concurrency guard

Before:

POST /api/forgetting/run → run_cycle() synchronously → blocking the event loop

After:

lock = app.state.forgetting_lock   (asyncio.Lock, created at app startup)

if lock.locked():
    return {"status": "already_running", "dry_run": dry_run}

async with lock:
    report = await asyncio.to_thread(run_cycle, dry_run)
    return serialised report

Two things improved: the cycle no longer blocks the event loop (moved to thread pool), and overlapping requests are rejected.

Finding — lock check is not strictly atomic but safe in practice: The if lock.locked() check is not atomic with the async with lock acquisition. In theory, two coroutines could both see locked()=False before either acquires. In CPython's asyncio with a single event loop, this cannot happen because coroutines don't preempt each other between these two lines — the event loop only switches at await points, and there's no await between the check and the async with. However, in a multi-worker deployment (e.g., uvicorn --workers 4), each worker process has its own lock and this guard provides no cross-process protection. This is acceptable for the stated goal (single-flight per process) but worth noting in a comment.

The asyncio.to_thread offload is the right pattern — it unblocks the event loop while the CPU-bound forgetting cycle runs.

---

7. ProceduralStore.record_outcome — memory.updated emission

Before:

record_outcome(record_id, success):
  record = get_by_id(record_id)
  if record is None: return
  record.record_outcome(success)
  self.replace(record)
  # ← no event emitted, state changed silently

After:

record_outcome(record_id, success):
  record = get_by_id(record_id)
  if record is None: return
  record.record_outcome(success)
  self.replace(record)
  self._emit_event("memory.updated", {
      record_id, memory_type, update_type: "procedural_outcome",
      success, success_count, failure_count, total_outcomes, wilson_score
  })

Clean addition. The event fires after replace() succeeds, so subscribers observe consistent state. The event payload is rich — contains the full outcome stats, not just a flag.

---

8. EventRecorder and ConsoleLogger — memory.updated wiring

Both were updated to subscribe to memory.updated. The additions are minimal and symmetric with existing event wiring. The ConsoleLogger.on_memory_updated format is consistent with on_memory_stored.

---

Test Coverage Assessment

Change	Test coverage
Superseded filtering in retrieve	test_retrieve_filters_superseded_records_by_default ✓
Admin opt-in include_superseded=True	test_retrieve_can_include_superseded_records_for_admin_flows ✓
Contradiction check status in write response	test_semantic_write_swallows_valueerror_from_contradiction_lookup updated ✓
Broad exception catch → status: "error"	test_semantic_write_reports_programming_error_from_contradiction_lookup ✓
Forgetting report status: "completed"	Integration tests updated ✓
Single-flight lock already_running	test_overlapping_forgetting_requests_return_already_running ✓
Post-resolve retrieval filters superseded	test_retrieval_hides_superseded_semantic_records_by_default ✓
memory.updated event from record_outcome	test_record_outcome_emits_memory_updated_event ✓
Logger on_memory_updated format	test_console_logger_formats_events updated ✓

Coverage is thorough. Every behavioral change has a corresponding test.

One gap: the FakeStore.get_all_records in test_forgetting_service.py now handles include_superseded but there is no explicit test asserting that the forgetting service's _scan_records passes include_superseded=True to the semantic store. The behavior is covered transitively through integration tests, but a unit test asserting the scan includes superseded semantic records would close this gap completely.

---

Final Judgement

Verdict: Approve with minor notes

The PR correctly addresses all four issues from #59:

1. ✅ Superseded semantic memories filtered from default recall — correct and consistent across retrieve, retrieve_by_vector, and get_all_records
2. ✅ Contradiction check status is now explicit (completed / skipped / error) instead of an empty list
3. ✅ Forgetting cycle single-flighted with a per-process async lock
4. ✅ memory.updated event emitted from record_outcome and wired into recorder + logger

The critical internal consistency fixes (_scan_records and find_likely_duplicates_batch opting into superseded records) are correct and necessary — without them, the new default filtering would have silently broken forgetting and duplicate detection.

Minor issues to be aware of:

• retrieve_by_vector with include_superseded=False now scans the full collection on every query (see note on semantic_store.py:113-115). Fine at current scale; could become a bottleneck later.
• The bare except Exception in _safe_contradiction_lookup downgrades programming errors from HTTP 500 to silent status: "error" in the response body — consider adding a server-side log line.
• The asyncio.Lock guard is per-process only; multi-worker deployments won't benefit.
  | Branch

[Atharva-Kanherkar]

@claude is this issue mergeable now?

[claude]

Claude finished @Atharva-Kanherkar's task in 52s —— View job

---

Mergeability Assessment

• Check CI status
• Verify minor issues from previous review were addressed
• Give mergeability verdict

---

Yes — this PR is ready to merge.

CI

Both checks pass (Tests ✅, Claude Code Review ✅). Vercel preview is Ready.

Previous Review Follow-ups

The "Tighten issue 59 review follow-ups" commit addressed all three minor issues flagged in the earlier review:

Issue	Status
retrieve_by_vector O(N) scan with no explanation	✅ Added comment at semantic_store.py:113 explaining why n_results = collection.count() and the scaling trade-off
except Exception in _safe_contradiction_lookup silently swallowed programming errors	✅ Added logger.exception(...) call — errors now appear in server logs with full traceback
asyncio.Lock per-process with no documentation	✅ Added explicit comment noting single-flight is per-process and that multi-worker deployments need an external coordinator
_serialise_forgetting_report missing status field	✅ Added "status": "completed" to the serialised report

All four behavioral changes from #59 are correct and fully tested. No blocking issues remain.
| Branch