From e32456d33762f65590b290cde988209655de9509 Mon Sep 17 00:00:00 2001 From: Aari Date: Tue, 14 Jul 2026 07:40:49 +0800 Subject: [PATCH] fix(memory): coerce stored confidence in the three remaining raw reads (#4034) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(memory): coerce stored confidence in the three remaining raw reads `_coerce_source_confidence` exists because `memory.json` is user-editable and written across versions: a `confidence` that is null, a str, a bool, out of range, or non-finite still has to read back as a usable score. Consolidation already routes every stored read through it, and #4023 did the same for the max_facts trim. Three reads still take the field raw. `_build_staleness_section` formats it with `f"{conf:.2f}"` — a str raises ValueError, None raises TypeError. `_do_update_memory_sync`'s `except Exception` swallows that into `return False`, aborting the whole memory-update cycle permanently, since the offending fact is then never rewritten. The staleness cap's `sort(key=lambda f: f.get("confidence", 0))` ranks the facts it is about to delete. A str raises the same way; the values that don't raise mis-rank instead. `true`/`inf` outrank a genuine 0.9 and push it into the removal slot; an absent key ranks 0 and is deleted first; `nan` compares false against everything, so which fact dies depends on where the corrupted one happens to sit in the file. `search_memory_facts` — the `memory_search` tool, added in #4023 — ranks results the same raw way and then truncates to `limit`, so the same mis-ranking hands the model the wrong facts, or fails the tool call outright on a str. All three now read through the helper, so an unusable stored confidence ranks as unknown (0.5): neither kept ahead of a real score nor evicted before one. The two sorts run in opposite directions, and 0.5 is load-bearing in both. * test(memory): anchor the confidence delta at every coerced read The str-based tests raise on main, so they cannot go red for any stored confidence that mis-ranks without raising. `true`/`false`/`inf` and an absent key never reached the except handler at all: bool subclasses int, so `true` ranked 1.0; `inf` outranked every real score; an absent key ranked 0. Silent fact loss in the staleness cap, wrong results out of `memory_search` — no log line either way. Parametrize both ranking sorts over the four non-raising inputs that fall to the 0.5 default, each pitted against a genuine neighbour chosen so the survivor flips. The sorts run in opposite directions, so the one matrix covers eviction from both ends. `nan` is excluded from those: its old rank is undefined rather than pinned to an end of the order, so one fixed input order happens to yield the correct survivor. Its real property is order-independence, asserted across both fact orders. The staleness prompt formatter is pinned by rendered value, not merely by not raising: 1.5 → 1.00, -0.3 → 0.00, and inf/nan/true/None → 0.50, matching the consolidation prompt that reads the same field through the same helper. --- backend/tests/test_memory_staleness_review.py | 180 ++++++++++++++++++ backend/tests/test_memory_updater.py | 89 +++++++++ 2 files changed, 269 insertions(+) diff --git a/backend/tests/test_memory_staleness_review.py b/backend/tests/test_memory_staleness_review.py index 186850002..e353e4d75 100644 --- a/backend/tests/test_memory_staleness_review.py +++ b/backend/tests/test_memory_staleness_review.py @@ -12,6 +12,8 @@ Covers: from datetime import UTC, datetime, timedelta from unittest.mock import MagicMock, patch +import pytest + from deerflow.agents.memory.updater import ( MemoryUpdater, _build_staleness_section, @@ -24,6 +26,10 @@ from deerflow.config.memory_config import MemoryConfig # ── Helpers ──────────────────────────────────────────────────────────────── +_ABSENT = object() +"""Sentinel: the fact carries no ``confidence`` key at all.""" + + def _memory_config(**overrides: object) -> MemoryConfig: config = MemoryConfig() for key, value in overrides.items(): @@ -244,6 +250,65 @@ class TestBuildStalenessSection: assert 'pref<"erences>' not in section assert "pref<"erences>" in section + @pytest.mark.parametrize("stored_confidence", ["0.9", None, "high", ""]) + def test_non_float_confidence_does_not_raise(self, stored_confidence): + """A stored ``confidence`` that is not a float must not abort the update. + + ``memory.json`` is user-editable and written across versions, which is why + ``_coerce_source_confidence`` exists. Formatting it raw raises ValueError on + a str and TypeError on None; ``_do_update_memory_sync``'s ``except Exception`` + turns that into a silent ``return False`` that aborts the whole memory-update + cycle -- permanently, since the offending fact is then never rewritten. + """ + fact = _make_fact("fact_x", "Some fact", "knowledge", 0.9, days_ago=120) + fact["confidence"] = stored_confidence + + section = _build_staleness_section([fact], 90) + + assert "fact_x" in section + assert "Some fact" in section + + @pytest.mark.parametrize( + ("stored_confidence", "rendered"), + [ + ("0.9", "0.90"), + ("high", "0.50"), + (None, "0.50"), + (True, "0.50"), + (1.5, "1.00"), + (-0.3, "0.00"), + (float("inf"), "0.50"), + (float("nan"), "0.50"), + ], + ) + def test_confidence_is_normalised_like_every_other_stored_read(self, stored_confidence, rendered): + """Pins the mapping, not just the absence of a crash. + + ``0.5`` is this module's default for an unknown confidence + (``create_memory_fact``, ``_normalize_memory_update_fact``, + ``_coerce_source_confidence``). Before this change the staleness prompt + rendered ``1.5`` as ``1.50``, ``inf`` as ``inf``, and a ``True`` as ``1.00`` + -- disagreeing with the consolidation prompt, which reads the same field + through the same helper. + """ + fact = _make_fact("fact_x", "Some fact", "knowledge", 0.9, days_ago=120) + fact["confidence"] = stored_confidence + + section = _build_staleness_section([fact], 90) + + assert f"| {rendered} |" in section + + def test_missing_confidence_key_renders_unknown_not_zero(self): + """An absent key is *unknown* (0.50), not *worthless* (0.00). + + The staleness cap removes the lowest-confidence facts first, so ranking an + unreadable confidence at 0.00 would make that fact the first one deleted. + """ + fact = _make_fact("fact_x", "Some fact", "knowledge", 0.9, days_ago=120) + del fact["confidence"] + + assert "| 0.50 |" in _build_staleness_section([fact], 90) + # ── _apply_updates with staleness removals ───────────────────────────────── @@ -353,6 +418,121 @@ class TestApplyUpdatesStaleness: assert "fact_mid" in remaining_ids assert "fact_low1" in remaining_ids + def test_safety_cap_sort_survives_non_float_stored_confidence(self): + """The cap's ranking sort reads stored confidence and must coerce it. + + ``sort(key=lambda f: f.get("confidence", 0))`` compares a str against a + float and raises ``TypeError``, which the caller swallows into an aborted + update. Fixing only the prompt formatter would move this crash rather than + remove it, so the sort is pinned here too. ``"0.95"`` must rank like 0.95. + """ + updater = MemoryUpdater() + current_memory = _make_memory( + [ + _make_fact("fact_str_high", confidence="0.95", days_ago=100), + _make_fact("fact_mid", confidence=0.80, days_ago=100), + _make_fact("fact_low", confidence=0.60, days_ago=100), + ] + ) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [ + {"id": "fact_str_high", "reason": "outdated"}, + {"id": "fact_mid", "reason": "outdated"}, + {"id": "fact_low", "reason": "outdated"}, + ], + } + + with patch( + "deerflow.agents.memory.updater.get_memory_config", + return_value=_memory_config(max_facts=100, staleness_max_removals_per_cycle=1), + ): + result = updater._apply_updates(current_memory, update_data) + + # Only the single lowest-confidence fact is removed; the string "0.95" + # ranks as the highest and survives. + remaining_ids = {f["id"] for f in result["facts"]} + assert remaining_ids == {"fact_str_high", "fact_mid"} + + @pytest.mark.parametrize( + ("stored_confidence", "rival_confidence", "survivor"), + [ + (_ABSENT, 0.1, "fact_x"), + (False, 0.1, "fact_x"), + (True, 0.9, "fact_rival"), + (float("inf"), 0.9, "fact_rival"), + ], + ) + def test_safety_cap_ranks_unusable_confidence_as_unknown(self, stored_confidence, rival_confidence, survivor): + """The cap deletes the lowest-ranked fact, so a mis-ranked one deletes its neighbour. + + Mirrors the max_facts trim's delta set with the sort reversed: here a + ``true``/``inf`` fact ranked *above* a genuine 0.9 and pushed it into + the removal slot. None of these raised under the old key, so the + string-coercion test above passes unchanged for all four. + """ + fact_x = _make_fact("fact_x", days_ago=100) + if stored_confidence is _ABSENT: + del fact_x["confidence"] + else: + fact_x["confidence"] = stored_confidence + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [ + {"id": "fact_x", "reason": "outdated"}, + {"id": "fact_rival", "reason": "outdated"}, + ], + } + + with patch( + "deerflow.agents.memory.updater.get_memory_config", + return_value=_memory_config(max_facts=100, staleness_max_removals_per_cycle=1), + ): + result = MemoryUpdater()._apply_updates( + _make_memory([fact_x, _make_fact("fact_rival", confidence=rival_confidence, days_ago=100)]), + update_data, + ) + + assert [f["id"] for f in result["facts"]] == [survivor] + + def test_safety_cap_with_nan_confidence_is_order_independent(self): + """Under the raw key the cap deleted either fact depending on their file order. + + ``nan`` compares false against every score, so ``sort`` leaves the pair + untouched: with the corrupted fact stored *second*, the genuine 0.9 one + landed in the removal slot instead. + """ + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [ + {"id": "fact_nan", "reason": "outdated"}, + {"id": "fact_rival", "reason": "outdated"}, + ], + } + + survivors = [] + for nan_first in (True, False): + nan_fact = _make_fact("fact_nan", confidence=float("nan"), days_ago=100) + rival = _make_fact("fact_rival", confidence=0.9, days_ago=100) + facts = [nan_fact, rival] if nan_first else [rival, nan_fact] + with patch( + "deerflow.agents.memory.updater.get_memory_config", + return_value=_memory_config(max_facts=100, staleness_max_removals_per_cycle=1), + ): + result = MemoryUpdater()._apply_updates(_make_memory(facts), update_data) + survivors.append(result["facts"][0]["id"]) + + assert survivors == ["fact_rival", "fact_rival"] + def test_empty_stale_removals_no_effect(self): updater = MemoryUpdater() current_memory = _make_memory( diff --git a/backend/tests/test_memory_updater.py b/backend/tests/test_memory_updater.py index 4c065ba0d..fe1ceebc6 100644 --- a/backend/tests/test_memory_updater.py +++ b/backend/tests/test_memory_updater.py @@ -2,6 +2,8 @@ import asyncio import threading from unittest.mock import AsyncMock, MagicMock, patch +import pytest + from deerflow.agents.memory.prompt import format_conversation_for_update from deerflow.agents.memory.updater import ( MemoryUpdater, @@ -14,6 +16,7 @@ from deerflow.agents.memory.updater import ( create_memory_fact_with_created_fact, delete_memory_fact, import_memory_data, + search_memory_facts, update_memory_fact, ) from deerflow.config.memory_config import MemoryConfig @@ -38,6 +41,10 @@ def _make_memory(facts: list[dict[str, object]] | None = None) -> dict[str, obje } +_ABSENT = object() +"""Sentinel: the fact carries no ``confidence`` key at all.""" + + def _memory_config(**overrides: object) -> MemoryConfig: config = MemoryConfig() for key, value in overrides.items(): @@ -255,6 +262,88 @@ def test_apply_updates_preserves_threshold_and_max_facts_trimming() -> None: assert result["facts"][1]["source"] == "thread-9" +def _searchable_fact(fact_id: str, confidence: object = _ABSENT) -> dict[str, object]: + fact: dict[str, object] = { + "id": fact_id, + "content": f"deploy runbook {fact_id}", + "category": "context", + "createdAt": "2026-03-18T00:00:00Z", + "source": "t", + } + if confidence is not _ABSENT: + fact["confidence"] = confidence + return fact + + +def test_search_memory_facts_sort_survives_non_float_stored_confidence() -> None: + """``memory_search`` ranks stored facts by confidence and must coerce it. + + ``sort(key=lambda f: f.get("confidence", 0))`` compares a str against a float + and raises ``TypeError``, which surfaces to the model as a failed tool call. + The stored ``"0.95"`` must rank as 0.95 and lead the results. + """ + facts = [ + _searchable_fact("f_low", 0.10), + _searchable_fact("f_str", "0.95"), + _searchable_fact("f_mid", 0.50), + ] + + with patch("deerflow.agents.memory.updater.get_memory_data", return_value=_make_memory(facts=facts)): + result = search_memory_facts("deploy runbook") + + assert [fact["id"] for fact in result] == ["f_str", "f_mid", "f_low"] + + +@pytest.mark.parametrize( + ("stored_confidence", "rival_confidence", "top"), + [ + # Ranked at the bottom by the old ``f.get("confidence", 0)`` key ... + (_ABSENT, 0.1, "f_x"), + (False, 0.1, "f_x"), + # ... and at the very top, because ``bool`` subclasses ``int`` and + # ``inf`` compares above every real score. (``nan`` also falls to the + # default, but its old ranking was order-dependent rather than pinned + # to an end — see the order-independence test below.) + (True, 0.9, "f_rival"), + (float("inf"), 0.9, "f_rival"), + ], +) +def test_search_memory_facts_ranks_unusable_confidence_as_unknown(stored_confidence, rival_confidence, top) -> None: + """Every value that falls to the 0.5 default must rank as *unknown*, not best or worst. + + The old key never raised on these — it silently mis-ranked them, so the + string-coercion test above cannot go red for any of them. ``true``/``inf`` + outranked a genuine 0.9 and pushed the better fact out of a capped result + set; a missing key ranked below a genuine 0.1 and dropped itself. ``limit`` + turns either mis-ranking into a wrong answer for the model. + """ + facts = [_searchable_fact("f_x", stored_confidence), _searchable_fact("f_rival", rival_confidence)] + + with patch("deerflow.agents.memory.updater.get_memory_data", return_value=_make_memory(facts=facts)): + result = search_memory_facts("deploy runbook", limit=1) + + assert [fact["id"] for fact in result] == [top] + + +def test_search_memory_facts_with_nan_confidence_is_order_independent() -> None: + """``nan`` compares false against everything, so a raw key leaves the sort undefined. + + Which fact the model gets back then depends on where the corrupted one happens + to sit in ``memory.json`` — the same file answers the same query differently + across two runs. Coercing ``nan`` to the 0.5 default restores a total order. + """ + tops = [] + for nan_first in (True, False): + nan_fact = _searchable_fact("f_nan", float("nan")) + rival = _searchable_fact("f_rival", 0.9) + facts = [nan_fact, rival] if nan_first else [rival, nan_fact] + with patch("deerflow.agents.memory.updater.get_memory_data", return_value=_make_memory(facts=facts)): + result = search_memory_facts("deploy runbook", limit=1) + tops.append(result[0]["id"]) + + assert tops == ["f_rival", "f_rival"] + + def test_apply_updates_preserves_source_error() -> None: updater = MemoryUpdater() current_memory = _make_memory()