From 4f9e00a6a52abbe2f6a435bd1eabcad332e6fa67 Mon Sep 17 00:00:00 2001 From: Shay Date: Wed, 20 May 2026 19:59:21 -0700 Subject: [PATCH] fix(cognition): bound speculative-subject cache + evict on COHERENT promotion (#85) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes audit Finding 5 (2026-05-20). Pre-fix ``CognitiveTurnPipeline._speculative_subjects`` was a bare ``set[str]`` that only grew over a session. Two correctness gaps: * A subject promoted to ``EpistemicStatus.COHERENT`` via the teaching review loop kept appearing with the "(speculative, not yet reviewed)" marker forever, contaminating reviewed material on later probes. * Long teaching sessions widened the per-turn substring scan in ``_should_mark_speculative`` without bound. Fix: * Back the cache with ``OrderedDict[str, None]`` (LRU) capped at ``_MAX_SPECULATIVE_SUBJECTS = 64``. * Introduce ``_remember_speculative_subject`` (insert / refresh) and ``_forget_speculative_subject`` (evict) helpers; route all SPECULATIVE inserts through them. * When a proposal lands as ``EpistemicStatus.COHERENT``, evict the subject and every long-enough non-stopword token derived from it, so the marker stops appearing on reviewed material. Iteration order in ``_should_mark_speculative`` is unchanged (keys view); lookups remain O(1). No surface change for any case the prior behavior didn't already mishandle, so byte-identical eval surfaces stay stable (verified locally against ``core eval cognition`` public / holdout / dev splits — all unchanged from MEMORY baseline). Tests (7 new, ``tests/test_speculative_subject_lifecycle.py``): * storage is an OrderedDict and the cap is 64 * remember normalizes (lower+strip) and drops empty input * remember refreshes LRU position on re-insert * cache caps at 64 with insertion-order eviction * forget is case-insensitive and removes the entry * forget on a missing / empty subject is a no-op * ``_should_mark_speculative`` triggers after remember and stops triggering after forget Audit findings referenced: https://github.com/AssetOverflow/core/pull/76 (Finding 5, "Unbounded ``_speculative_subjects``") --- core/cognition/pipeline.py | 75 +++++++++++++++-- tests/test_speculative_subject_lifecycle.py | 93 +++++++++++++++++++++ 2 files changed, 159 insertions(+), 9 deletions(-) create mode 100644 tests/test_speculative_subject_lifecycle.py diff --git a/core/cognition/pipeline.py b/core/cognition/pipeline.py index d76dcdcc..46df449d 100644 --- a/core/cognition/pipeline.py +++ b/core/cognition/pipeline.py @@ -15,6 +15,8 @@ Constraint: ChatRuntime.chat() and ChatResponse contract are unchanged. from __future__ import annotations +from collections import OrderedDict + from field.state import FieldState from core.cognition.result import CognitiveTurnResult from core.cognition.surface_resolution import resolve_surface @@ -75,6 +77,15 @@ _SUBJECT_STOPWORDS: frozenset[str] = frozenset({ "your", "their", "answer", }) +# Finding 5 (audit 2026-05-20) — cap the speculative-subjects cache so a +# long teaching session cannot grow it without bound. 64 is large enough +# to cover every distinct teaching subject a single session realistically +# emits and small enough that the per-turn substring scan in +# ``_should_mark_speculative`` stays trivially cheap. LRU eviction: a +# subject re-encountered as SPECULATIVE refreshes its position; coherent +# promotion removes it explicitly. +_MAX_SPECULATIVE_SUBJECTS = 64 + class CognitiveTurnPipeline: """Thin pipeline wrapper over ChatRuntime. @@ -94,7 +105,16 @@ class CognitiveTurnPipeline: # (by subject substring or reflexive query shape), the surface # is prefixed with _SPECULATIVE_SURFACE_MARKER so the user can # tell ratified knowledge from unreviewed teaching material. - self._speculative_subjects: set[str] = set() + # + # Finding 5 (audit 2026-05-20) — backed by an OrderedDict so the + # cache is bounded (LRU, cap ``_MAX_SPECULATIVE_SUBJECTS``) and + # supports explicit eviction when a proposal is promoted to + # COHERENT. Pre-fix this was a bare ``set`` that only grew, + # which both leaked speculative markers onto reviewed subjects + # forever and widened the per-turn substring scan unboundedly. + # Iteration order matches insertion / refresh order; lookups + # remain O(1). + self._speculative_subjects: OrderedDict[str, None] = OrderedDict() # ------------------------------------------------------------------ # Public API @@ -204,19 +224,28 @@ class CognitiveTurnPipeline: # proposal.subject (often a fragment of the correction text); # also split-and-add each ≥4-char token so prefixed parses like # "correction: wisdom" still match a probe about "wisdom". - if proposal is not None and proposal.epistemic_status is EpistemicStatus.SPECULATIVE: + if proposal is not None: sources: list[str] = [] if proposal.triple is not None and proposal.triple[0]: sources.append(proposal.triple[0]) if proposal.subject: sources.append(proposal.subject) - for src in sources: - lowered = src.lower().strip() - if lowered: - self._speculative_subjects.add(lowered) - for tok in _SUBJECT_SPLIT_RE.split(src.lower()): - if len(tok) >= 4 and tok not in _SUBJECT_STOPWORDS: - self._speculative_subjects.add(tok) + if proposal.epistemic_status is EpistemicStatus.SPECULATIVE: + for src in sources: + self._remember_speculative_subject(src) + for tok in _SUBJECT_SPLIT_RE.split(src.lower()): + if len(tok) >= 4 and tok not in _SUBJECT_STOPWORDS: + self._remember_speculative_subject(tok) + elif proposal.epistemic_status is EpistemicStatus.COHERENT: + # Finding 5 (audit 2026-05-20) — once teaching review + # promotes a proposal to COHERENT, the subject is no + # longer speculative; evict its tokens so the marker + # stops appearing on subsequent probes about it. + for src in sources: + self._forget_speculative_subject(src) + for tok in _SUBJECT_SPLIT_RE.split(src.lower()): + if len(tok) >= 4 and tok not in _SUBJECT_STOPWORDS: + self._forget_speculative_subject(tok) # Advance turn counter and remember surface for next correction binding self._turn_number += 1 @@ -348,6 +377,34 @@ class CognitiveTurnPipeline: ) return ratify_intent(intent, prompt_versor, vocab=vocab) + def _remember_speculative_subject(self, subject: str) -> None: + """Add (or refresh LRU position of) a speculative subject token. + + Finding 5 (audit 2026-05-20). Caps the cache at + ``_MAX_SPECULATIVE_SUBJECTS`` via insertion-order eviction. + Empty / whitespace-only inputs are dropped silently so callers + can pass raw fragments without guarding. + """ + subject = subject.lower().strip() + if not subject: + return + self._speculative_subjects.pop(subject, None) + self._speculative_subjects[subject] = None + while len(self._speculative_subjects) > _MAX_SPECULATIVE_SUBJECTS: + self._speculative_subjects.popitem(last=False) + + def _forget_speculative_subject(self, subject: str) -> None: + """Evict a subject from the speculative-marker cache. + + Called when a SPECULATIVE proposal is promoted to COHERENT via + the teaching review loop, so reviewed material stops being + marked speculative on later probes. No-op if the subject is + not present. + """ + subject = subject.lower().strip() + if subject: + self._speculative_subjects.pop(subject, None) + def _should_mark_speculative(self, text: str, surface: str) -> bool: """Decide whether ``surface`` should carry the SPECULATIVE marker. diff --git a/tests/test_speculative_subject_lifecycle.py b/tests/test_speculative_subject_lifecycle.py new file mode 100644 index 00000000..c3f345b8 --- /dev/null +++ b/tests/test_speculative_subject_lifecycle.py @@ -0,0 +1,93 @@ +"""Speculative-subject cache lifecycle (Finding 5, audit 2026-05-20). + +Pre-fix ``CognitiveTurnPipeline._speculative_subjects`` was an unbounded +``set[str]`` that only grew over a session. Two correctness gaps: + + * A subject promoted to ``EpistemicStatus.COHERENT`` via the teaching + review loop kept appearing with the "(speculative, not yet reviewed)" + marker forever. + * Long teaching sessions widened the per-turn substring scan inside + ``_should_mark_speculative`` without bound. + +These tests pin the lifecycle so the fix cannot silently regress. +""" + +from __future__ import annotations + +from collections import OrderedDict + +import pytest + +from chat.runtime import ChatRuntime +from core.cognition import CognitiveTurnPipeline +from core.cognition.pipeline import _MAX_SPECULATIVE_SUBJECTS + + +@pytest.fixture() +def pipeline() -> CognitiveTurnPipeline: + return CognitiveTurnPipeline(runtime=ChatRuntime()) + + +def test_storage_is_bounded_ordereddict(pipeline: CognitiveTurnPipeline) -> None: + assert isinstance(pipeline._speculative_subjects, OrderedDict) + assert _MAX_SPECULATIVE_SUBJECTS == 64 + + +def test_remember_inserts_and_normalizes(pipeline: CognitiveTurnPipeline) -> None: + pipeline._remember_speculative_subject(" TRUTH ") + assert "truth" in pipeline._speculative_subjects + pipeline._remember_speculative_subject("") + pipeline._remember_speculative_subject(" ") + # Empty / whitespace inputs are silently dropped + assert list(pipeline._speculative_subjects) == ["truth"] + + +def test_remember_refreshes_lru_position(pipeline: CognitiveTurnPipeline) -> None: + pipeline._remember_speculative_subject("alpha") + pipeline._remember_speculative_subject("beta") + pipeline._remember_speculative_subject("alpha") + # alpha was re-touched after beta, so alpha is now the MRU entry + assert list(pipeline._speculative_subjects) == ["beta", "alpha"] + + +def test_cache_caps_at_max(pipeline: CognitiveTurnPipeline) -> None: + for i in range(_MAX_SPECULATIVE_SUBJECTS + 10): + pipeline._remember_speculative_subject(f"subj{i:03d}") + assert len(pipeline._speculative_subjects) == _MAX_SPECULATIVE_SUBJECTS + # Oldest 10 entries (subj000..subj009) evicted; newest survive + keys = list(pipeline._speculative_subjects) + assert "subj000" not in keys + assert "subj009" not in keys + assert "subj010" in keys + assert keys[-1] == f"subj{_MAX_SPECULATIVE_SUBJECTS + 9:03d}" + + +def test_forget_removes(pipeline: CognitiveTurnPipeline) -> None: + pipeline._remember_speculative_subject("truth") + pipeline._remember_speculative_subject("light") + pipeline._forget_speculative_subject("Truth") # case-insensitive + assert "truth" not in pipeline._speculative_subjects + assert "light" in pipeline._speculative_subjects + + +def test_forget_missing_is_noop(pipeline: CognitiveTurnPipeline) -> None: + pipeline._remember_speculative_subject("truth") + pipeline._forget_speculative_subject("never-added") + pipeline._forget_speculative_subject("") + assert list(pipeline._speculative_subjects) == ["truth"] + + +def test_should_mark_speculative_still_iterates_correctly( + pipeline: CognitiveTurnPipeline, +) -> None: + """The marker decision still triggers on stored subjects.""" + pipeline._remember_speculative_subject("wisdom") + assert pipeline._should_mark_speculative( + text="Tell me about wisdom", + surface="Wisdom is the application of knowledge.", + ) + pipeline._forget_speculative_subject("wisdom") + assert not pipeline._should_mark_speculative( + text="Tell me about wisdom", + surface="Wisdom is the application of knowledge.", + )