From fb51f82da6e7326a6a06162a717af9f3a438d9e9 Mon Sep 17 00:00:00 2001 From: cyre Date: Fri, 7 Aug 2026 06:10:26 +0800 Subject: [PATCH] fix(recall): exclude superseded lessons from lessons.jsonl retrieval recall.py's _load_structured() already deduped lessons.jsonl by latest row per id (handling retraction, which appends a same-id row rather than editing in place) but had no supersession awareness: supersession creates a NEW id whose supersedes field points at the old one, and the old row's own status stays "accepted" forever since supersession never edits it. Proactive recall could return both the stale and replacement guidance for the same topic. render_lessons.py's _build_auto_section already computed exactly this -- an old-id -> new-id map, accepted-supersessions-only (a provisional --supersedes must not retire the old lesson before its replacement is itself accepted). Extracted that computation into a public superseded_by_map(lessons) function and import it from recall.py, rather than re-deriving the same rule a second time -- retrieval and rendering now share one source of truth for "which lessons are currently retired" instead of two copies that can drift apart. Tests: tests/test_recall_supersession.py (superseded lesson excluded from _load_structured, recall() ranks the replacement not the superseded lesson, provisional supersession does not retire the old lesson) + a direct unit test on superseded_by_map itself. Full suite: 179 passed, 1 pre-existing unrelated failure (confirmed via git stash against a clean checkout: test_unknown_executable_is_a_structured_start_failure, a macOS PermissionError-vs-FileNotFoundError platform quirk, not touched by this change). --- .agent/memory/render_lessons.py | 24 +++-- .agent/tools/recall.py | 11 ++ tests/test_recall_supersession.py | 161 ++++++++++++++++++++++++++++++ 3 files changed, 190 insertions(+), 6 deletions(-) create mode 100644 tests/test_recall_supersession.py diff --git a/.agent/memory/render_lessons.py b/.agent/memory/render_lessons.py index d01abf4..ec8fdb8 100644 --- a/.agent/memory/render_lessons.py +++ b/.agent/memory/render_lessons.py @@ -132,12 +132,19 @@ def _bullet_for(lesson, superseded_by): return f"- {claim} " -def _build_auto_section(lessons): - # Only accepted supersessions flip the old lesson to strikethrough. - # A provisional --supersedes would otherwise blank the active lesson - # before its replacement has been accepted, leaving no active guidance - # on that topic at all (retrieval skips both provisional and - # strikethrough). +def superseded_by_map(lessons): + """Old lesson id -> id of the accepted lesson that supersedes it. + + Only accepted supersessions retire the old lesson. A provisional + --supersedes would otherwise blank the active lesson before its + replacement has itself been accepted, leaving no active guidance on + that topic at all -- callers that skip provisional/strikethrough + entries would then surface nothing for that topic. + + Shared with recall.py so retrieval and rendering never disagree on + which lessons are currently retired -- see its docstring for the bug + this fixed (recall returning both the stale and replacement guidance). + """ superseded_by = {} for L in lessons: if L.get("status") != "accepted": @@ -145,6 +152,11 @@ def _build_auto_section(lessons): sup = L.get("supersedes") if sup: superseded_by[sup] = L.get("id") + return superseded_by + + +def _build_auto_section(lessons): + superseded_by = superseded_by_map(lessons) groups = defaultdict(list) for L in lessons: diff --git a/.agent/tools/recall.py b/.agent/tools/recall.py index 98dfd26..071cbd9 100644 --- a/.agent/tools/recall.py +++ b/.agent/tools/recall.py @@ -25,7 +25,9 @@ BASE = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) sys.path.insert(0, os.path.join(BASE, "harness")) +sys.path.insert(0, os.path.join(BASE, "memory")) from text import word_set # noqa: E402 +from render_lessons import superseded_by_map # noqa: E402 LESSONS_JSONL = os.path.join(BASE, "memory/semantic/lessons.jsonl") LESSONS_MD = os.path.join(BASE, "memory/semantic/LESSONS.md") @@ -68,8 +70,17 @@ def _load_structured(): order.append(lid) latest[lid] = row + # A lesson can stay status="accepted" on its own row even after being + # superseded -- supersession creates a NEW id, it never edits the old + # row. Exclude ids an accepted supersession retires, mirroring + # render_lessons.py's rendering rule, so recall doesn't return both the + # stale and replacement guidance for the same topic. + retired = superseded_by_map(latest.values()) + out = [] for lid in order: + if lid in retired: + continue lesson = latest[lid] if lesson.get("status") != "accepted": continue diff --git a/tests/test_recall_supersession.py b/tests/test_recall_supersession.py new file mode 100644 index 0000000..fbddbb4 --- /dev/null +++ b/tests/test_recall_supersession.py @@ -0,0 +1,161 @@ +"""recall.py must not surface superseded lessons alongside their replacement. + +lessons.jsonl is append-only: supersession writes a NEW id whose +`supersedes` field points at the old one -- it never edits the old row. +The old row keeps `status: "accepted"` forever, so a naive per-row +`status == "accepted"` filter (recall's own dedupe already handles same-id +retraction, but not this different-id case) returns both the stale and +the replacement guidance for the same topic. Fixed by sharing +`render_lessons.superseded_by_map` between rendering and retrieval so +they never disagree about which lessons are currently retired. +""" +import importlib.util +import json +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +RECALL = ROOT / ".agent" / "tools" / "recall.py" +RENDER_LESSONS = ROOT / ".agent" / "memory" / "render_lessons.py" + + +def load_module(path: Path, module_name: str): + spec = importlib.util.spec_from_file_location(module_name, path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def _lesson(lesson_id, claim, conditions, status="accepted", supersedes=None): + return { + "id": lesson_id, + "claim": claim, + "conditions": conditions, + "status": status, + "supersedes": supersedes, + "accepted_at": "2026-08-01T00:00:00+00:00", + } + + +class RecallSupersessionTest(unittest.TestCase): + def with_semantic_dir(self): + tmp = tempfile.TemporaryDirectory() + semantic_dir = Path(tmp.name) / ".agent" / "memory" / "semantic" + semantic_dir.mkdir(parents=True) + return tmp, semantic_dir + + def write_lessons(self, semantic_dir: Path, lessons): + path = semantic_dir / "lessons.jsonl" + with path.open("w", encoding="utf-8") as f: + for lesson in lessons: + f.write(json.dumps(lesson) + "\n") + + def load_recall(self, semantic_dir: Path, module_name: str): + recall = load_module(RECALL, module_name) + recall.LESSONS_JSONL = str(semantic_dir / "lessons.jsonl") + recall.LESSONS_MD = str(semantic_dir / "missing.md") + return recall + + def test_superseded_lesson_is_excluded_from_structured_load(self): + tmp, semantic_dir = self.with_semantic_dir() + self.addCleanup(tmp.cleanup) + old_id, new_id = "lesson_old_git_tip", "lesson_new_git_tip" + self.write_lessons( + semantic_dir, + [ + _lesson(old_id, "git branch -f silently no-ops on checked-out branch", + ["git branch -f"]), + _lesson(new_id, "git branch -f refuses with exit 128 on checked-out branch", + ["git branch -f", "rollback"], supersedes=old_id), + ], + ) + recall = self.load_recall(semantic_dir, "recall_1") + + loaded = recall._load_structured() + ids = [row["id"] for row in loaded] + + self.assertIn(new_id, ids) + self.assertNotIn(old_id, ids) + + def test_recall_ranks_replacement_not_the_superseded_lesson(self): + tmp, semantic_dir = self.with_semantic_dir() + self.addCleanup(tmp.cleanup) + old_id, new_id = "lesson_old_git_tip", "lesson_new_git_tip" + self.write_lessons( + semantic_dir, + [ + _lesson( + old_id, + "git branch -f silently no-ops when checked out", + ["git branch -f", "cherry-pick loop"], + ), + _lesson( + new_id, + "git branch -f refuses to move the checked-out " + "branch; use git update-ref then git restore", + ["git branch -f", "git update-ref", "rollback"], + supersedes=old_id, + ), + ], + ) + recall = self.load_recall(semantic_dir, "recall_2") + + result, meta = recall.recall( + "git branch -f cherry-pick rollback update-ref", top_k=5, min_score=0.01 + ) + returned_ids = [row.get("id") for row in result if row.get("id")] + + self.assertIn(new_id, returned_ids) + self.assertNotIn(old_id, returned_ids) + self.assertEqual(meta["considered"], 1) + + def test_provisional_supersession_does_not_retire_old_lesson(self): + tmp, semantic_dir = self.with_semantic_dir() + self.addCleanup(tmp.cleanup) + old_id, provisional_id = "lesson_active", "lesson_provisional" + self.write_lessons( + semantic_dir, + [ + _lesson(old_id, "Keep using the established rollback procedure", + ["rollback"]), + _lesson( + provisional_id, + "Experimental rollback procedure under review", + ["rollback"], + status="provisional", + supersedes=old_id, + ), + ], + ) + recall = self.load_recall(semantic_dir, "recall_3") + + loaded = recall._load_structured() + ids = [row["id"] for row in loaded] + + self.assertIn(old_id, ids) + self.assertNotIn(provisional_id, ids) + + +class SupersededByMapTest(unittest.TestCase): + """render_lessons.superseded_by_map is the single source of truth both + rendering and recall share -- exercised directly so drift between the + two consumers is caught here, not just at the recall integration level. + """ + + def test_only_accepted_supersessions_are_mapped(self): + render_lessons = load_module(RENDER_LESSONS, "render_lessons_map_test") + lessons = [ + _lesson("lesson_a", "a", []), + _lesson("lesson_b", "b", [], supersedes="lesson_a"), + _lesson("lesson_c", "c", []), + _lesson("lesson_d", "d", [], status="provisional", supersedes="lesson_c"), + ] + + mapping = render_lessons.superseded_by_map(lessons) + + self.assertEqual(mapping, {"lesson_a": "lesson_b"}) + + +if __name__ == "__main__": + unittest.main()