From 9c8a0a752a158834c2fbd522970ab2ce8af4f37b Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Fri, 7 Aug 2026 12:04:27 -0500 Subject: [PATCH 01/19] test(recall): fail loudly when a test resolves into the live channel store Ref #955 -- closes at promotion. Channel tests that exercise the post path without isolating resolution fall through to tier-2 resolution and write fixture messages into the real home-level channel store. The override to prevent that already existed; what was missing is that nothing FAILED when a test reached the real store, so the contamination was silent and cumulative. Root cause found while building this: four test files "isolate" themselves by setting SYNAPT_PROJECT_DIR or SYNAPT_DATA_DIR. Neither name is read anywhere in the package. Setting them produces no error and no warning, so the tests have looked isolated for their whole lifetime while resolving to the live store. An isolation step that cannot fail is not an isolation step. Two layers: Layer 1 covers the import window only. pytest_configure runs before any test module is imported, so module-level and collection-time code needs somewhere safe to resolve; an autouse fixture then hands ordinary resolution back per test. That narrowness was learned, not designed -- leaving a session-wide SYNAPT_SHARED_CHANNELS_DIR in place broke thirty tests, because the override is tier 1 and outranks the tier-3 local resolution some tests deliberately exercise, and one shared directory turns independent tests into shared-state ones. An environment default is a semantic change to path resolution, not a neutral safety net. Layer 2 refuses without redirecting, so it does not change where anything legitimately resolves. Production gains a no-op policy seam consulted before any store directory is created or file opened; with no policy installed behaviour is unchanged. The seam sits at resolution, not only at the write calls: _channels_dir mkdirs the global directory as part of resolving it, so a guard at the append seam would report a write rather than prevent one. Five write surfaces are covered, including one that composes the store path itself and so is reachable by neither the resolver guard nor the explicit-argument guard. Also isolates the fourteen tests the guard caught resolving to the live store, switching them to the override the resolver actually consults. Measured on one runner, whole suite, BOTH SIDES AT BASE 1245f62: base 1245f62, no guard 23 failed / 2768 passed this commit, guard armed 23 failed / 2792 passed Net new failures zero. The +24 passed is exactly the 24 witnesses this commit adds. The 23 failures are pre-existing at this base and untouched here. An earlier draft of this message quoted 2774 passed and claimed one incidental fix. Both came from runs taken at the previous base and did not survive re-measurement here: the failure that appeared "fixed" was already fixed upstream. Numbers from two different bases cannot be one delta, and a pair that does not reconcile is a pair to re-measure rather than to explain. The data-root half ships behind --strict-recall-data-root, off by default. Arming it fails 30 further tests across 10 files, and those are true positives: recall tests deliberately strip the root override to measure path inference, and that inference currently lands in a real checkout. Isolating those sites is separate reviewable work; the mechanism ships built and witnessed rather than absent, with the count reported rather than quietly carried. Premium boundary: recall is OSS -- this is test infrastructure plus a neutral, no-op policy seam on local channel and data path resolution. No identity, no org semantics, no entitlement behaviour. --- conftest.py | 406 ++++++++++++++ src/synapt/recall/channel.py | 55 +- src/synapt/recall/core.py | 31 ++ tests/recall/test_action_registry.py | 39 ++ tests/recall/test_channel_hooks.py | 6 +- tests/recall/test_dm_channels.py | 24 +- tests/recall/test_store_isolation_guard.py | 596 +++++++++++++++++++++ tests/recall_store_isolation.py | 215 ++++++++ 8 files changed, 1364 insertions(+), 8 deletions(-) create mode 100644 conftest.py create mode 100644 tests/recall/test_store_isolation_guard.py create mode 100644 tests/recall_store_isolation.py diff --git a/conftest.py b/conftest.py new file mode 100644 index 00000000..5f9c581e --- /dev/null +++ b/conftest.py @@ -0,0 +1,406 @@ +"""Session-wide on-disk isolation for the recall test suite. + +Ref #955 — closes at promotion. + +Channel tests that exercise the post path without isolating resolution fall +through to tier-2 resolution and write their fixture messages into the real +home-level store. The environment override to prevent that already existed; +what was missing is that *nothing failed* when a test reached the real store, +so the contamination was silent and cumulative. + +This file installs the policy in two layers: + +* **Layer 1, the import window only** — ``pytest_configure`` runs before any + test module is imported, so module-level code and collection-time helpers + need somewhere safe to resolve. An autouse fixture then hands ordinary + resolution back to each test. +* **Layer 2, at the seams** — refusal. The resolver and every write surface + consult the policy before creating or opening anything. + +Layer 1 is deliberately narrow, and that narrowness was learned rather than +designed. Leaving a session-wide ``SYNAPT_SHARED_CHANNELS_DIR`` in place for +the tests themselves broke thirty of them: the override is tier 1 and outranks +the tier-3 local resolution some tests deliberately exercise, and one shared +directory turned independent tests into shared-state ones. An environment +default is a *semantic* change to path resolution, not a neutral safety net. + +So the guarantee rests on Layer 2, which refuses without redirecting. A harness +that changes what the suite measures has stopped being a harness. + +This lives at the repository root rather than under ``tests/`` on purpose: a +root ``conftest.py`` is always an *initial* conftest, so its ``pytest_configure`` +is guaranteed to run before collection begins regardless of which path +arguments pytest was invoked with. A conftest inside ``tests/`` is loaded +during collection, which is the ordering the guard exists to get ahead of. +""" + +from __future__ import annotations + +import os +import shutil +import sys +import tempfile +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent / "tests")) + +from recall_store_isolation import ( # noqa: E402 + RecallStoreIsolationError, + StoreIsolationPolicy, + contained_by, + protected_channel_root, +) + +POLICY = StoreIsolationPolicy() + + +# --------------------------------------------------------------------------- +# options and markers +# --------------------------------------------------------------------------- + +def pytest_addoption(parser): + parser.addoption( + "--allow-live-channel-store-tests", + action="store_true", + default=False, + help=( + "Authorize tests marked live_channel_store to touch the real " + "home-level channel store. Integration use only; never in the " + "ordinary CI command." + ), + ) + parser.addoption( + "--strict-recall-data-root", + action="store_true", + default=False, + help=( + "Also refuse implicit recall data paths (journal, index, archive, " + "knowledge) that resolve outside a pytest-owned root. Off by " + "default: the suite currently has real sites that resolve into the " + "operator's live store, and arming this before they are isolated " + "would turn a true finding into a red suite." + ), + ) + + +# --------------------------------------------------------------------------- +# Layer 1 — safe defaults, installed before collection +# --------------------------------------------------------------------------- + +def pytest_configure(config): + config.addinivalue_line( + "markers", + "live_channel_store: test deliberately touches the real channel store; " + "requires --allow-live-channel-store-tests", + ) + + # Under the system temp directory, not the repository. A session root + # inside the checkout would leave an untracked directory after every run, + # and "no dirty workspaces" exists so that dirty state stays meaningful — + # a harness that manufactures noise every run trains people to ignore it. + session_root = Path(tempfile.mkdtemp(prefix="recall-store-isolation-")) + channel_root = session_root / "channels" + data_root = session_root / "data" + channel_root.mkdir(parents=True, exist_ok=True) + # SYNAPT_RECALL_ROOT refuses a root that does not exist, so create it + # before pointing at it rather than letting the first reader discover a + # mistyped path as an empty history. + data_root.mkdir(parents=True, exist_ok=True) + POLICY.session_root = session_root + + POLICY.session_channel_root = channel_root + POLICY.session_data_root = data_root + POLICY.allow_live = config.getoption("--allow-live-channel-store-tests") + + # Only supply a default; an invocation that already chose a test root keeps it. + os.environ.setdefault("SYNAPT_SHARED_CHANNELS_DIR", str(channel_root)) + os.environ.setdefault("SYNAPT_RECALL_ROOT", str(data_root)) + # The companion override matters here: redirecting only the root would file + # this run's per-worktree data under the cwd basename inside the shared + # store, which is another workspace's namespace rather than our own. + os.environ.setdefault("SYNAPT_RECALL_WORKTREE", "pytest-isolated") + + _install_policy(config.getoption("--strict-recall-data-root")) + + +def _install_policy(strict_data_root: bool): + """Arm the guards. + + The channel guard is always on — that is the contract this harness exists + for. It refuses without redirecting, so it does not change where anything + legitimately resolves. + + The data-root guard is opt-in *for now*, and the reason is worth stating + plainly rather than hiding behind a default. Arming it fails 30 further + tests across 10 files, and those failures are TRUE POSITIVES: + ``tests/recall`` deliberately strips the root override so tests measure + path inference, and that inference currently lands in a real checkout's + ``.synapt/recall``. Isolating those sites is real work and a separate + reviewable change. Shipping it armed would mean either a red suite or 30 + edits buried in a guard PR; shipping it absent would lose the mechanism. So + it ships built, witnessed, and one flag away — and the count is reported + rather than quietly carried. + + That count is 30 measured at base 1245f62, not the ~60 an earlier draft + carried. The larger figure was taken while this file still installed a + session-wide channel override, which was itself breaking 30 tests; half the + "true positives" were the harness's own doing. A measurement inherits the + configuration it was taken under, and a number quoted after the + configuration changed is a stale claim wearing a precise costume. + """ + from synapt.recall import channel as channel_mod + from synapt.recall import core as core_mod + + channel_mod.set_store_path_policy(POLICY.check_channel_path) + if strict_data_root: + core_mod.set_data_root_policy(POLICY.check_data_root) + + +def pytest_unconfigure(config): + from synapt.recall import channel as channel_mod + from synapt.recall import core as core_mod + + channel_mod.set_store_path_policy(None) + core_mod.set_data_root_policy(None) + + # Removing the session root is safe only because the guard already proved + # it is outside every protected root — re-checked here rather than assumed, + # since this is the one place the harness deletes anything. + root = POLICY.session_root + if root is None: + return + if any(contained_by(root, protected) for protected in POLICY.roots()): + return + shutil.rmtree(root, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# current-item tracking +# --------------------------------------------------------------------------- +# The refusal names the test that caused it, and the live-store opt-in needs the +# item's markers. Plain phase hooks are used rather than a wrapper so this stays +# correct across pytest's hookwrapper API changes. + +def pytest_runtest_setup(item): + POLICY.current_item = item + + +def pytest_runtest_call(item): + POLICY.current_item = item + + +def pytest_runtest_teardown(item, nextitem): + POLICY.current_item = item + + +def pytest_runtest_logfinish(nodeid, location): + POLICY.current_item = None + + +# --------------------------------------------------------------------------- +# fixtures +# --------------------------------------------------------------------------- + +@pytest.fixture(autouse=True) +def _natural_resolution_per_test(monkeypatch): + """Hand each test back its ordinary resolution, and let the guard do the work. + + The session defaults set in ``pytest_configure`` cover *import and + collection* time, when no fixture has run yet. They are deliberately not + left in place for the tests themselves, for two reasons found empirically + rather than reasoned in advance: + + * ``SYNAPT_SHARED_CHANNELS_DIR`` is tier 1 and outranks everything. Tests + that build their own project directory and expect tier-3 local + resolution (``TestMessageFormatValidation``) then write somewhere else + and cannot find their own file. + * A single session-wide root also converts independent tests into + shared-state tests — ``test_join_spam`` asserted one join event and saw + three, then five, then six as earlier tests piled into the same JSONL. + + Both are the same mistake in different clothes: an environment default is a + *semantic* change to path resolution, not a neutral safety net. So Layer 1 + narrows to the window it is actually needed for, and the guarantee moves + entirely to Layer 2 — which refuses the real store without altering where + anything legitimately resolves. A harness that changes what the suite + measures has stopped being a harness. + """ + monkeypatch.delenv("SYNAPT_SHARED_CHANNELS_DIR", raising=False) + monkeypatch.delenv("SYNAPT_RECALL_ROOT", raising=False) + monkeypatch.delenv("SYNAPT_RECALL_WORKTREE", raising=False) + + +@pytest.fixture(autouse=True) +def _unwind_extra_protected_roots(): + """Decoy roots never outlive the test that registered them.""" + yield + POLICY.clear_extra_roots() + + +@pytest.fixture(name="protected_channel_root") +def _protected_channel_root() -> Path: + return protected_channel_root() + + +@pytest.fixture +def register_protected_root(): + """Add a decoy protected root for the duration of one test. + + Adds only. The real home store is permanent in the policy and cannot be + removed by any fixture, so registering a decoy buys a witness without + buying a bypass. + """ + def _register(root: Path) -> Path: + POLICY.register_extra_root(Path(root)) + return Path(root) + + return _register + + +@pytest.fixture +def isolation_policy(): + """The live policy object, for witnesses that inspect the harness itself.""" + return POLICY + + +@pytest.fixture +def store_isolation_error(): + return RecallStoreIsolationError + + +@pytest.fixture +def strict_data_root(): + """Arm the data-root guard for one test. + + The witnesses for the data-root half must exercise the policy whether or + not the suite was invoked with ``--strict-recall-data-root``, otherwise the + mechanism ships with tests that silently no-op in the default CI command — + which is the "check that cannot fail" shape this whole harness is against. + """ + from synapt.recall import core as core_mod + + previous = core_mod.set_data_root_policy(POLICY.check_data_root) + try: + yield POLICY + finally: + core_mod.set_data_root_policy(previous) + + +@pytest.fixture +def rederive_protected_root(): + """Re-derive the protected root *now*, under whatever the test has patched. + + Returned as a callable rather than a value so the derivation runs after the + test's monkeypatching, which is the only way to witness that the boundary + does not move when HOME and ``Path.home()`` do. + """ + return protected_channel_root + + +pytest_plugins = ["pytester"] + + +_NESTED_CONFTEST = ''' +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, {tests_dir!r}) +from recall_store_isolation import StoreIsolationPolicy + +POLICY = StoreIsolationPolicy() +DECOY = Path(__file__).parent / "decoy-home" / ".synapt" / "channels" + + +def pytest_addoption(parser): + parser.addoption( + "--allow-live-channel-store-tests", action="store_true", default=False + ) + + +_PREVIOUS = [] + + +def pytest_configure(config): + config.addinivalue_line("markers", "live_channel_store: touches the live store") + POLICY.register_extra_root(DECOY) + POLICY.allow_live = config.getoption("--allow-live-channel-store-tests") + from synapt.recall import channel as channel_mod + # Save and RESTORE rather than clearing: pytester runs in-process, so this + # nested run shares module globals with the parent session. Setting the + # policy to None on unconfigure would disarm the outer guard for every test + # that follows, and nothing would report it. + _PREVIOUS.append(channel_mod.set_store_path_policy(POLICY.check_channel_path)) + + +def pytest_unconfigure(config): + from synapt.recall import channel as channel_mod + channel_mod.set_store_path_policy(_PREVIOUS.pop() if _PREVIOUS else None) + + +def pytest_runtest_setup(item): + POLICY.current_item = item + + +def pytest_runtest_call(item): + POLICY.current_item = item + + +@pytest.fixture +def decoy_root(): + return DECOY +''' + +_NESTED_TEST = ''' +from synapt.recall import channel as channel_mod + +{decorator} +def test_writes_to_the_live_store(decoy_root): + msg = channel_mod.ChannelMessage( + timestamp="2026-08-07T00:00:00Z", + channel="dev", + type="message", + body="deliberate live write", + from_agent="s_optin", + ) + channel_mod._append_message(msg, channels_dir=decoy_root) + assert (decoy_root / "dev.jsonl").exists() +''' + + +def _build_nested(pytester, decorator: str): + """Scaffold a nested pytest run that installs the *real* policy. + + The mark and the option are pytest-level facts, so the only faithful way to + witness their interaction is to run pytest. Importing the shared policy + module rather than restating the rule keeps this a test of the guard rather + than a test of a second implementation of the guard. + """ + tests_dir = str(Path(__file__).parent / "tests") + pytester.makeconftest(_NESTED_CONFTEST.format(tests_dir=tests_dir)) + pytester.makepyfile(_NESTED_TEST.format(decorator=decorator)) + return pytester + + +@pytest.fixture +def pytester_isolated(pytester): + """A nested run whose single test carries the live_channel_store mark.""" + return _build_nested(pytester, "@__import__('pytest').mark.live_channel_store") + + +@pytest.fixture +def pytester_isolated_unmarked(pytester): + """A nested run whose single test carries no mark.""" + return _build_nested(pytester, "") + + +@pytest.fixture +def isolated_channels(tmp_path, monkeypatch) -> Path: + """A per-test channel root, stronger than the session default.""" + channels = tmp_path / "channels" + channels.mkdir() + monkeypatch.setenv("SYNAPT_SHARED_CHANNELS_DIR", str(channels)) + return channels diff --git a/src/synapt/recall/channel.py b/src/synapt/recall/channel.py index d4ba0202..c2101eb5 100644 --- a/src/synapt/recall/channel.py +++ b/src/synapt/recall/channel.py @@ -58,6 +58,40 @@ _message_posted_hooks: list[Callable[["ChannelMessage", "Path | None"], None]] = [] +# --------------------------------------------------------------------------- +# Store-path policy seam +# --------------------------------------------------------------------------- +# A consumer may install a callable that is consulted with (operation, path) +# immediately before any channel-store directory is created or any channel +# file is opened for writing. With no policy installed this is a no-op and +# behaviour is unchanged; the seam exists so a harness can refuse a resolved +# path *before* the write rather than detect it afterwards. +# +# The seam sits at resolution, not only at the write calls, because resolving +# the global store directory creates it as a side effect — by the time an +# append is attempted the directory already exists. + +_store_path_policy: Callable[[str, "Path"], None] | None = None + + +def set_store_path_policy( + policy: Callable[[str, "Path"], None] | None, +) -> Callable[[str, "Path"], None] | None: + """Install a channel-store path policy; returns the previous one.""" + global _store_path_policy + previous = _store_path_policy + _store_path_policy = policy + return previous + + +def _guard_store_path(operation: str, path: Path) -> Path: + """Consult the installed policy, then return *path* unchanged.""" + policy = _store_path_policy + if policy is not None: + policy(operation, path) + return path + + def register_message_hook( hook: Callable[["ChannelMessage", "Path | None"], None], ) -> None: @@ -196,14 +230,18 @@ def _channels_dir(project_dir: Path | None = None) -> Path: # Tier 1: explicit env var override shared = _shared_channels_dir() if shared: - return shared + return _guard_store_path("resolve_channels_dir", shared) # Tier 2: global store from manifest URL global_dir = _global_channels_dir(project_dir) if global_dir: + # Guarded before the mkdir: resolving this tier creates the directory, + # so a check placed after it would be reporting a write, not preventing + # one. + _guard_store_path("resolve_channels_dir", global_dir) global_dir.mkdir(parents=True, exist_ok=True) return global_dir # Tier 3: local fallback - return _local_channels_dir(project_dir) + return _guard_store_path("resolve_channels_dir", _local_channels_dir(project_dir)) def _channel_to_filename(channel: str) -> str: @@ -1080,6 +1118,9 @@ def _append_message( path = channels_dir / f"{_channel_to_filename(msg.channel)}.jsonl" else: path = _channel_path(msg.channel, project_dir) + # An explicit channels_dir bypasses resolution entirely, so the write + # surface is guarded in its own right rather than trusting the resolver. + _guard_store_path("append_message", path) path.parent.mkdir(parents=True, exist_ok=True) from synapt.recall._filelock import lock_exclusive with open(path, "a", encoding="utf-8") as f: @@ -1260,6 +1301,10 @@ def _copy_attachments( ) -> list[str]: """Copy attachments into the channel store and return relative paths.""" target_dir = _attachments_dir(project_dir) / message_id + # Attachments are a channel-owned write surface. A scrub or audit that + # enumerates only *.jsonl misses this tree by construction, so it is + # guarded explicitly rather than inheriting the JSONL path's coverage. + _guard_store_path("copy_attachments", target_dir) target_dir.mkdir(parents=True, exist_ok=True) stored: list[str] = [] @@ -3072,6 +3117,10 @@ def migrate_channels_to_global( return target_dir = global_dir / org_id / project_id + # This function composes the store path itself — it neither calls + # _channels_dir nor accepts a channels_dir, so it sits outside both of the + # forms the other guards cover and needs its own. + _guard_store_path("migrate_channels_to_global", target_dir) target_dir.mkdir(parents=True, exist_ok=True) # Migrate JSONL channel files @@ -3134,6 +3183,8 @@ def _migrate_cursors( ) -> None: """Migrate cursor data from local channels.db to global _state.db.""" state_db = global_dir / "_state.db" + # Reachable directly, not only via migrate_channels_to_global. + _guard_store_path("migrate_cursors", state_db) state_db.parent.mkdir(parents=True, exist_ok=True) # Read local cursors diff --git a/src/synapt/recall/core.py b/src/synapt/recall/core.py index c3638ff7..3a7c4e04 100644 --- a/src/synapt/recall/core.py +++ b/src/synapt/recall/core.py @@ -4468,6 +4468,32 @@ def _worktree_name(project_dir: Path | None = None) -> str: return (project_dir or Path.cwd()).resolve().name +# --------------------------------------------------------------------------- +# Data-root policy seam +# --------------------------------------------------------------------------- +# A consumer may install a callable consulted with (operation, path) when a +# recall data root is resolved, before any directory is created or migrated. +# With no policy installed this is a no-op and behaviour is unchanged. + +_data_root_policy = None + + +def set_data_root_policy(policy): + """Install a recall data-root policy; returns the previous one.""" + global _data_root_policy + previous = _data_root_policy + _data_root_policy = policy + return previous + + +def _guard_data_root(operation: str, path: Path) -> Path: + """Consult the installed policy, then return *path* unchanged.""" + policy = _data_root_policy + if policy is not None: + policy(operation, path) + return path + + def project_data_dir(project_dir: Path | None = None) -> Path: """Return the root synapt recall data directory. @@ -4530,6 +4556,11 @@ def project_data_dir(project_dir: Path | None = None) -> Path: new_dir = root / ".synapt" / "recall" + # Guarded before the legacy-migration tail below, which renames real + # directories on disk. A consumer may install a policy that refuses a + # resolved data root; with none installed this is a no-op. + _guard_data_root("project_data_dir", new_dir) + if not new_dir.exists(): # Tier 1: .synapse/recall/ (intermediate rename era) mid_dir = root / ".synapse" / "recall" diff --git a/tests/recall/test_action_registry.py b/tests/recall/test_action_registry.py index 68f2feab..7f4a776a 100644 --- a/tests/recall/test_action_registry.py +++ b/tests/recall/test_action_registry.py @@ -7,10 +7,36 @@ All tests are expected to FAIL until the implementation lands. """ +import os +import tempfile import unittest +from dataclasses import dataclass +from pathlib import Path from unittest.mock import MagicMock, patch +@dataclass +class _ChannelRootPatch: + """A restorable ``SYNAPT_SHARED_CHANNELS_DIR`` override.""" + + previous: str | None + + def restore(self) -> None: + if self.previous is None: + os.environ.pop("SYNAPT_SHARED_CHANNELS_DIR", None) + else: + os.environ["SYNAPT_SHARED_CHANNELS_DIR"] = self.previous + + +def _isolated_channel_root() -> _ChannelRootPatch: + """Point channel resolution at a throwaway directory for one test.""" + previous = os.environ.get("SYNAPT_SHARED_CHANNELS_DIR") + os.environ["SYNAPT_SHARED_CHANNELS_DIR"] = str( + Path(tempfile.mkdtemp()) / "channels" + ) + return _ChannelRootPatch(previous) + + # OSS base actions that must always be available OSS_ACTIONS = { "join", "leave", "post", "read", "read_message", "who", @@ -49,6 +75,12 @@ def test_registry_has_dispatch_method(self): class TestOSSBaseActions(unittest.TestCase): """OSS base actions must be registered by default.""" + def setUp(self): + self._channels = _isolated_channel_root() + + def tearDown(self): + self._channels.restore() + def test_oss_actions_registered(self): """All OSS base actions should be in the default registry.""" from synapt.recall.actions import get_default_registry @@ -258,10 +290,17 @@ def test_truly_unknown_action_shows_as_unknown(self): class TestRecallChannelIntegration(unittest.TestCase): """The live MCP tool should dispatch through the shared action registry.""" + def setUp(self): + # These dispatch through the real channel post path. With no override + # they resolve to the home-level store and their fixture messages land + # in live channels (Ref #955). + self._channels = _isolated_channel_root() + def tearDown(self): from synapt.recall.actions import reset_action_registry reset_action_registry() + self._channels.restore() def test_recall_channel_uses_registry_dispatch(self): """recall_channel should route OSS actions through the shared registry.""" diff --git a/tests/recall/test_channel_hooks.py b/tests/recall/test_channel_hooks.py index 9641cf5b..fb6c3fb9 100644 --- a/tests/recall/test_channel_hooks.py +++ b/tests/recall/test_channel_hooks.py @@ -28,7 +28,9 @@ class TestMessageHooks(unittest.TestCase): def setUp(self): self._tmp = tempfile.mkdtemp() - os.environ["SYNAPT_DATA_DIR"] = self._tmp + # SYNAPT_DATA_DIR is read nowhere in the package; it isolated nothing and + # these tests appended into the real channel store (Ref #955). + os.environ["SYNAPT_SHARED_CHANNELS_DIR"] = self._tmp # Save original hooks self._original_hooks = list(_message_posted_hooks) @@ -37,7 +39,7 @@ def tearDown(self): _clear_message_hooks() for hook in self._original_hooks: register_message_hook(hook) - os.environ.pop("SYNAPT_DATA_DIR", None) + os.environ.pop("SYNAPT_SHARED_CHANNELS_DIR", None) def test_default_hooks_registered(self): """Default mention and wake hooks should be registered at import time.""" diff --git a/tests/recall/test_dm_channels.py b/tests/recall/test_dm_channels.py index d5deea8d..da3de230 100644 --- a/tests/recall/test_dm_channels.py +++ b/tests/recall/test_dm_channels.py @@ -52,8 +52,12 @@ class TestDMPostAndRead(unittest.TestCase): def setUp(self): self.tmpdir = tempfile.mkdtemp() + # SYNAPT_PROJECT_DIR is read nowhere in the package, so it isolated + # nothing — these tests resolved to the real home-level channel store. + # SYNAPT_SHARED_CHANNELS_DIR is the override the resolver actually + # consults (Ref #955). self._env_patch = patch.dict(os.environ, { - "SYNAPT_PROJECT_DIR": self.tmpdir, + "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), }) self._env_patch.start() @@ -109,8 +113,12 @@ class TestDMPrivacy(unittest.TestCase): def setUp(self): self.tmpdir = tempfile.mkdtemp() + # SYNAPT_PROJECT_DIR is read nowhere in the package, so it isolated + # nothing — these tests resolved to the real home-level channel store. + # SYNAPT_SHARED_CHANNELS_DIR is the override the resolver actually + # consults (Ref #955). self._env_patch = patch.dict(os.environ, { - "SYNAPT_PROJECT_DIR": self.tmpdir, + "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), }) self._env_patch.start() @@ -174,8 +182,12 @@ class TestDMDiscovery(unittest.TestCase): def setUp(self): self.tmpdir = tempfile.mkdtemp() + # SYNAPT_PROJECT_DIR is read nowhere in the package, so it isolated + # nothing — these tests resolved to the real home-level channel store. + # SYNAPT_SHARED_CHANNELS_DIR is the override the resolver actually + # consults (Ref #955). self._env_patch = patch.dict(os.environ, { - "SYNAPT_PROJECT_DIR": self.tmpdir, + "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), }) self._env_patch.start() @@ -245,8 +257,12 @@ class TestDMInRecallSearch(unittest.TestCase): def setUp(self): self.tmpdir = tempfile.mkdtemp() + # SYNAPT_PROJECT_DIR is read nowhere in the package, so it isolated + # nothing — these tests resolved to the real home-level channel store. + # SYNAPT_SHARED_CHANNELS_DIR is the override the resolver actually + # consults (Ref #955). self._env_patch = patch.dict(os.environ, { - "SYNAPT_PROJECT_DIR": self.tmpdir, + "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), }) self._env_patch.start() diff --git a/tests/recall/test_store_isolation_guard.py b/tests/recall/test_store_isolation_guard.py new file mode 100644 index 00000000..28571127 --- /dev/null +++ b/tests/recall/test_store_isolation_guard.py @@ -0,0 +1,596 @@ +"""Witnesses for the recall test-store isolation guard. + +Ref #955 — closes at promotion. + +The governing claim is narrow: a test cannot silently write to the real +home-level channel store, and an implicit recall data path cannot silently +resolve into a real checkout. These tests pin that claim. + +Two things are deliberately separated here: + +* the *boundary derivation* — that the protected root comes from the operating + system account and cannot be moved by a fixture — is pinned once, against + the real root, by reading only. +* the *refusal mechanics* — that a resolved candidate under a protected root + is refused before the write — are pinned against a decoy root registered for + the duration of a single test. + +That split exists so no witness has to attempt a write at the real store to +prove the guard prevents writes at the real store. A prevention witness that +contaminates when it fails is not a safety net; it is the defect with a test +wrapped around it. Registering a decoy *adds* a protected root and can never +remove the real one, so the split does not open a bypass. +""" + +from __future__ import annotations + +import os +import pathlib +from pathlib import Path + +import pytest + +from synapt.recall import channel as channel_mod +from synapt.recall import core as core_mod +from synapt.recall import direct as direct_mod +from synapt.recall import journal as journal_mod + + +# --------------------------------------------------------------------------- +# helpers +# --------------------------------------------------------------------------- + +def _entries(root: Path) -> set[str]: + """Every path under *root*, as a comparable set. + + Enumerates the whole tree, not ``*.jsonl``. The issue already has evidence + of fixture material below the attachments tree, so a JSONL-only audit + reports success by construction while missing the artifact. + """ + if not root.exists(): + return set() + return {str(p) for p in root.rglob("*")} + + +def _message(body: str) -> "channel_mod.ChannelMessage": + """A minimal ChannelMessage; timestamp and type have no defaults.""" + return channel_mod.ChannelMessage( + timestamp="2026-08-07T00:00:00Z", + channel="dev", + type="message", + body=body, + from_agent="s_witness", + ) + + +def _make_gripspace(tmp: Path, org: str = "decoy-org", repo: str = "decoy-repo") -> Path: + """Build a gripspace whose manifest resolves to /. + + Mirrors the real layout closely enough that ``_global_channels_dir`` + takes its tier-2 branch rather than falling through to tier 3. + """ + root = tmp / "gripspace" + manifest = root / ".gitgrip" / "spaces" / "main" + manifest.mkdir(parents=True) + (root / ".gitgrip" / "griptrees.json").write_text("{}", encoding="utf-8") + (manifest / "gripspace.yml").write_text( + f"manifest:\n url: git@github.com:{org}/{repo}.git\n", + encoding="utf-8", + ) + core_mod._gripspace_cache.clear() + return root + + +# --------------------------------------------------------------------------- +# W0 — the boundary derivation itself +# --------------------------------------------------------------------------- + +def test_protected_root_is_derived_from_the_os_account_not_from_patched_home( + protected_channel_root, rederive_protected_root, monkeypatch, tmp_path +): + """The protected boundary survives a fixture moving HOME and Path.home(). + + This is the one witness that must speak about the real root, and it does so + by reading only. If the boundary were derived from the value a test is + currently asking the code to use, every other witness here would be + circular: the test would move the boundary and then prove nothing crosses + it. + """ + import pwd + + account_home = Path(pwd.getpwuid(os.getuid()).pw_dir).resolve() + assert protected_channel_root == account_home / ".synapt" / "channels" + + monkeypatch.setenv("HOME", str(tmp_path / "fake-home")) + monkeypatch.setattr(pathlib.Path, "home", staticmethod(lambda: tmp_path / "fake-home")) + + # Re-derived under a fully patched home, the boundary must not move. + assert rederive_protected_root() == account_home / ".synapt" / "channels" + + +# --------------------------------------------------------------------------- +# W1 — the ordinary path is isolated, and the real store is untouched +# --------------------------------------------------------------------------- + +def test_ordinary_channel_post_lands_in_a_pytest_owned_directory( + protected_channel_root, tmp_path, monkeypatch +): + """An ordinary post writes under the session root and never the real store.""" + baseline = _entries(protected_channel_root) + + channels = tmp_path / "channels" + monkeypatch.setenv("SYNAPT_SHARED_CHANNELS_DIR", str(channels)) + + resolved = channel_mod._channels_dir() + assert resolved == channels + + channel_mod.channel_post("dev", "isolation witness", project_dir=tmp_path) + + written = channels / "dev.jsonl" + assert written.exists() + assert "isolation witness" in written.read_text(encoding="utf-8") + + assert _entries(protected_channel_root) == baseline + + +def test_the_guard_is_armed_with_no_per_test_fixture_at_all( + protected_channel_root, store_isolation_error +): + """A test that sets nothing up is still protected. + + This is the property that matters, and it is deliberately *not* "the + environment override is set." An earlier draft asserted exactly that, and + it was measuring the wrong thing: leaving a session-wide override in place + made tier 1 outrank the tier-3 resolution some tests deliberately exercise, + and turned independent tests into shared-state ones. The override is a + semantic change to path resolution; the guard is not. So what a + fixture-less test inherits is the guard, not a redirect. + """ + assert channel_mod._store_path_policy is not None + + with pytest.raises(store_isolation_error): + # Pointed at the protected root explicitly rather than relying on this + # machine happening to resolve there. + channel_mod._guard_store_path("witness", protected_channel_root / "dev.jsonl") + + +def test_layer_one_covers_the_import_window(isolation_policy): + """The pre-collection default exists, even though tests do not run under it. + + ``pytest_configure`` runs before any test module is imported, which is the + window no fixture can reach — module-level code and collection-time helpers + resolve there. The per-test fixture then hands resolution back. Both halves + are load-bearing and neither is visible from the other's vantage point. + """ + assert isolation_policy.session_channel_root is not None + assert isolation_policy.session_channel_root.exists() + assert isolation_policy.session_data_root is not None + + +# --------------------------------------------------------------------------- +# W2 — manifest-resolving gripspace with the override absent +# --------------------------------------------------------------------------- + +def test_gripspace_resolution_without_override_fails_before_creating_anything( + register_protected_root, store_isolation_error, tmp_path, monkeypatch +): + """Tier-2 resolution is refused before its own mkdir. + + ``_channels_dir`` mkdirs the global directory *as part of resolving it*, so + a guard installed only at the append seam would already be too late — the + directory would exist under the real store before anything tried to write a + message into it. + """ + decoy_home = tmp_path / "home" + decoy_root = decoy_home / ".synapt" / "channels" + register_protected_root(decoy_root) + + monkeypatch.setattr(pathlib.Path, "home", staticmethod(lambda: decoy_home)) + monkeypatch.delenv("SYNAPT_SHARED_CHANNELS_DIR", raising=False) + + gripspace = _make_gripspace(tmp_path) + before = _entries(decoy_root) + + with pytest.raises(store_isolation_error) as excinfo: + channel_mod._channels_dir(project_dir=gripspace) + + assert _entries(decoy_root) == before + assert not decoy_root.exists() + + message = str(excinfo.value) + assert "decoy-org" in message and "decoy-repo" in message + assert str(decoy_root) in message + assert "test_gripspace_resolution_without_override_fails" in message + + +def test_the_public_post_api_is_refused_end_to_end( + register_protected_root, store_isolation_error, tmp_path, monkeypatch +): + """The refusal survives the real entry point, not just the internal seam. + + ``channel.py`` contains seven broad ``except Exception`` handlers, and the + refusal is an ``AssertionError`` — which one of them would happily swallow, + leaving a test that posts into a protected store and reports success. The + other witnesses call the seam directly and so cannot detect that. This one + goes through ``channel_post``, the function real callers use. + """ + decoy_home = tmp_path / "home" + decoy_root = decoy_home / ".synapt" / "channels" + register_protected_root(decoy_root) + + monkeypatch.setattr(pathlib.Path, "home", staticmethod(lambda: decoy_home)) + monkeypatch.delenv("SYNAPT_SHARED_CHANNELS_DIR", raising=False) + + gripspace = _make_gripspace(tmp_path) + before = _entries(decoy_root) + + with pytest.raises(store_isolation_error): + channel_mod.channel_post("dev", "must not reach the store", project_dir=gripspace) + + assert _entries(decoy_root) == before + assert not decoy_root.exists() + + +def test_unsetting_the_override_mid_test_does_not_buy_a_bypass( + register_protected_root, store_isolation_error, tmp_path, monkeypatch +): + """A test that clears the environment still cannot reach the store. + + An env-var-only harness is bypassable by exactly this move, which is why + the refusal lives at the resolver rather than in the fixture that sets the + default. + """ + decoy_home = tmp_path / "home" + register_protected_root(decoy_home / ".synapt" / "channels") + monkeypatch.setattr(pathlib.Path, "home", staticmethod(lambda: decoy_home)) + + gripspace = _make_gripspace(tmp_path) + os.environ.pop("SYNAPT_SHARED_CHANNELS_DIR", None) + + with pytest.raises(store_isolation_error): + channel_mod._channels_dir(project_dir=gripspace) + + +# --------------------------------------------------------------------------- +# W3 — an explicit channels_dir argument +# --------------------------------------------------------------------------- + +def test_explicit_channels_dir_at_the_store_fails_before_the_append( + register_protected_root, store_isolation_error, tmp_path +): + """The explicit-argument form bypasses resolution, so it is guarded separately.""" + decoy_root = tmp_path / "home" / ".synapt" / "channels" + register_protected_root(decoy_root) + + target = decoy_root / "decoy-org" / "decoy-repo" + before = _entries(decoy_root) + + msg = _message("must not be appended") + + with pytest.raises(store_isolation_error): + channel_mod._append_message(msg, channels_dir=target) + + assert _entries(decoy_root) == before + assert not (target / "dev.jsonl").exists() + + +# --------------------------------------------------------------------------- +# W4 — attachments +# --------------------------------------------------------------------------- + +def test_attachment_copy_into_the_store_fails_before_the_copy( + register_protected_root, store_isolation_error, tmp_path, monkeypatch +): + """Attachments are a channel-owned write surface and go through the same policy. + + A cleanup that enumerates only ``*.jsonl`` misses this path by + construction, which is precisely why it needs its own witness rather than + inheriting confidence from the JSONL one. + """ + decoy_home = tmp_path / "home" + decoy_root = decoy_home / ".synapt" / "channels" + register_protected_root(decoy_root) + + monkeypatch.setattr(pathlib.Path, "home", staticmethod(lambda: decoy_home)) + monkeypatch.setenv("SYNAPT_SHARED_CHANNELS_DIR", str(decoy_root)) + + source = tmp_path / "payload.txt" + source.write_text("attachment body", encoding="utf-8") + before = _entries(decoy_root) + + with pytest.raises(store_isolation_error): + channel_mod._copy_attachments("msg-witness", [str(source)]) + + assert _entries(decoy_root) == before + + +# --------------------------------------------------------------------------- +# W5 — channel-owned child paths other than .jsonl +# --------------------------------------------------------------------------- + +def test_direct_inbox_child_path_is_covered( + register_protected_root, store_isolation_error, tmp_path, monkeypatch +): + """The direct-message tree resolves through the same seam and is covered. + + Pinning only ``dev.jsonl`` would prove the guard for the file class the + defect was first noticed in and nothing else. + """ + decoy_home = tmp_path / "home" + register_protected_root(decoy_home / ".synapt" / "channels") + monkeypatch.setattr(pathlib.Path, "home", staticmethod(lambda: decoy_home)) + monkeypatch.delenv("SYNAPT_SHARED_CHANNELS_DIR", raising=False) + + gripspace = _make_gripspace(tmp_path) + + with pytest.raises(store_isolation_error): + direct_mod._direct_dir(project_dir=gripspace) + + +def test_local_to_global_migration_is_covered( + register_protected_root, store_isolation_error, tmp_path +): + """``migrate_channels_to_global`` composes the store path itself. + + It never calls ``_channels_dir`` and never takes a ``channels_dir`` + argument, so it sits outside both forms the design note enumerated. Killing + the named surfaces proves the named set; this is the pristine case that sat + outside the enumeration. + """ + decoy_root = tmp_path / "home" / ".synapt" / "channels" + register_protected_root(decoy_root) + + local = tmp_path / "local-channels" + local.mkdir() + (local / "dev.jsonl").write_text( + '{"channel":"dev","from_agent":"s_witness","body":"migrating","timestamp":"2026-08-07T00:00:00Z"}\n', + encoding="utf-8", + ) + before = _entries(decoy_root) + + with pytest.raises(store_isolation_error): + channel_mod.migrate_channels_to_global(local, decoy_root, "decoy-org", "decoy-repo") + + assert _entries(decoy_root) == before + + +# --------------------------------------------------------------------------- +# W6 / W7 — containment is a path property, not a string property +# --------------------------------------------------------------------------- + +def test_symlink_resolving_into_the_store_is_refused( + register_protected_root, store_isolation_error, tmp_path +): + """A candidate is resolved before it is judged.""" + decoy_root = tmp_path / "home" / ".synapt" / "channels" + decoy_root.mkdir(parents=True) + register_protected_root(decoy_root) + + link = tmp_path / "innocent-looking" + link.symlink_to(decoy_root, target_is_directory=True) + + msg = _message("via symlink") + before = _entries(decoy_root) + + with pytest.raises(store_isolation_error): + channel_mod._append_message(msg, channels_dir=link) + + assert _entries(decoy_root) == before + + +def test_sibling_with_a_textual_prefix_is_accepted( + register_protected_root, tmp_path, monkeypatch +): + """``channels-backup`` is not a descendant of ``channels``. + + A ``startswith`` check refuses this path and would be indistinguishable + from a correct guard on every other witness in this file. This is the case + that separates containment from string prefixing. + """ + decoy_root = tmp_path / "home" / ".synapt" / "channels" + register_protected_root(decoy_root) + + sibling = tmp_path / "home" / ".synapt" / "channels-backup" + assert str(sibling).startswith(str(decoy_root)) + + monkeypatch.setenv("SYNAPT_SHARED_CHANNELS_DIR", str(sibling)) + + resolved = channel_mod._channels_dir() + assert resolved == sibling + + channel_mod.channel_post("dev", "sibling is fine", project_dir=tmp_path) + assert (sibling / "dev.jsonl").exists() + + +def test_the_protected_root_itself_is_refused_not_only_its_children( + register_protected_root, store_isolation_error, tmp_path +): + """The boundary is closed, not half-open.""" + decoy_root = tmp_path / "home" / ".synapt" / "channels" + register_protected_root(decoy_root) + + msg = _message("at the root itself") + with pytest.raises(store_isolation_error): + channel_mod._append_message(msg, channels_dir=decoy_root) + + +# --------------------------------------------------------------------------- +# W8 — the deliberate live-store opt-in +# --------------------------------------------------------------------------- + +def test_the_mark_alone_is_not_permission(pytester_isolated): + """A marked test without the command-line option still fails. + + The mark records intent; the option records authorization. Collapsing them + would let a single decorator re-open the store. + """ + result = pytester_isolated.runpytest("-p", "no:cacheprovider") + result.assert_outcomes(failed=1) + result.stdout.fnmatch_lines(["*recall test isolation violation*"]) + + +def test_the_option_alone_is_not_permission(pytester_isolated_unmarked): + """An unmarked test does not become authorized because the flag was passed.""" + result = pytester_isolated_unmarked.runpytest( + "--allow-live-channel-store-tests", "-p", "no:cacheprovider" + ) + result.assert_outcomes(failed=1) + + +def test_mark_plus_option_authorizes_and_says_so_loudly(pytester_isolated): + """Both present: the write proceeds and the authorization is reported.""" + result = pytester_isolated.runpytest( + "--allow-live-channel-store-tests", "-p", "no:cacheprovider" + ) + result.assert_outcomes(passed=1) + result.stdout.fnmatch_lines(["*authorized to touch the live channel store*"]) + + +def test_the_default_failure_does_not_advertise_the_opt_in( + register_protected_root, store_isolation_error, tmp_path +): + """The first remedy offered must be isolation, never authorization. + + A failure message that leads with the bypass converts a safety guard into a + documented workaround, and the next person to hit it will reach for the + flag rather than fix the fixture. + """ + decoy_root = tmp_path / "home" / ".synapt" / "channels" + register_protected_root(decoy_root) + + msg = _message("x") + with pytest.raises(store_isolation_error) as excinfo: + channel_mod._append_message(msg, channels_dir=decoy_root) + + message = str(excinfo.value) + assert "SYNAPT_SHARED_CHANNELS_DIR" in message + assert "allow-live-channel-store-tests" not in message + + +# --------------------------------------------------------------------------- +# W9 — the recall data root (journal, index, archive, knowledge) +# --------------------------------------------------------------------------- + +def test_implicit_journal_resolution_cannot_silently_reach_a_real_checkout( + strict_data_root, store_isolation_error, +): + """``_journal_path()`` takes no project dir, so it follows cwd. + + This is the mechanism behind the live specimen: a CLI path that calls + ``_journal_path()`` without the test project reads whichever journal the + process working directory happens to supply. A real checkout supplies the + operator's journal; a fresh worktree supplies an empty one, and a negative + assertion silently changes result with no test-code change. + + Note the interaction with ``tests/recall/conftest.py``, which deliberately + strips ``SYNAPT_RECALL_ROOT`` so these tests measure path *inference* + rather than the override. That intent is right and is preserved here — but + inference must not be allowed to land somewhere real. So the contract under + the stripped-override regime is not "resolves pytest-owned"; it is + "refuses, loudly." Silence is the only outcome ruled out. + """ + with pytest.raises(store_isolation_error) as excinfo: + journal_mod._journal_path() + + message = str(excinfo.value) + assert "operation=project_data_dir" in message + assert "expected under=" in message + + +def test_journal_resolution_is_independent_of_the_working_directory( + strict_data_root, tmp_path, monkeypatch +): + """Identical bytes, two working directories, one resolution. + + The specimen's defect was not a wrong path — it was a path that *varied* + with where the process happened to be standing, which makes a passing run + and a failing run indistinguishable at the source level. With a + pytest-owned root supplied, the variance is gone. + """ + root = tmp_path / "owned" + root.mkdir() + monkeypatch.setenv("SYNAPT_RECALL_ROOT", str(root)) + monkeypatch.setenv("SYNAPT_RECALL_WORKTREE", "pytest-isolated") + core_mod._gripspace_cache.clear() + + from_repo = journal_mod._journal_path().resolve() + + elsewhere = tmp_path / "fresh-worktree" + elsewhere.mkdir() + monkeypatch.chdir(elsewhere) + core_mod._gripspace_cache.clear() + + from_elsewhere = journal_mod._journal_path().resolve() + assert from_elsewhere == from_repo + assert root.resolve() in from_repo.parents + + +def test_explicit_project_dir_outside_the_test_root_is_refused( + strict_data_root, store_isolation_error, monkeypatch +): + """An explicit project dir must still be proven test-owned. + + Otherwise a test can sidestep the channel guard entirely and still read or + write the real journal, making its assertions depend on operator history. + The repository checkout is used as the outside path because it is a real + one — a scratch directory would not exercise the case that matters. + """ + monkeypatch.delenv("SYNAPT_RECALL_ROOT", raising=False) + repo_checkout = Path(__file__).resolve().parents[2] + + with pytest.raises(store_isolation_error): + core_mod.project_data_dir(repo_checkout) + + +# --------------------------------------------------------------------------- +# The guard must not be disarmable by a nested run +# --------------------------------------------------------------------------- + +def test_guard_survives_a_nested_pytest_run(pytester_isolated, tmp_path): + """A nested pytest session must not leave the outer guard uninstalled. + + ``pytester`` runs in-process, so a nested conftest shares module globals + with this session. A nested ``pytest_unconfigure`` that cleared the policy + outright would disarm the guard for every test that ran afterwards, and + nothing anywhere would report it — the suite would stay green while the + protection it advertises was gone. Found exactly this way: three later + witnesses started failing with the wrong error after the first nested run + landed. + """ + pytester_isolated.runpytest("-p", "no:cacheprovider") + + from synapt.recall import channel as channel_mod + + assert channel_mod._store_path_policy is not None, ( + "the nested run left the channel-store guard uninstalled" + ) + + +def test_recall_root_override_redirects_the_data_dir(tmp_path, monkeypatch): + """The override exists and takes priority over cwd/worktree/gripspace resolution. + + Pinned on its own because Layer 1 depends on it: the design note specified + this override as the mechanism, but no such override existed in the + resolver, so the harness half it prescribed could not have worked. + """ + root = tmp_path / "pytest-owned-root" + root.mkdir() # the override refuses a root that does not exist, by design + monkeypatch.setenv("SYNAPT_RECALL_ROOT", str(root)) + core_mod._gripspace_cache.clear() + + assert core_mod.project_data_dir() == root / ".synapt" / "recall" + + +def test_recall_root_override_refuses_a_root_that_does_not_exist(tmp_path, monkeypatch): + """A mistyped root must not be minted as a fresh empty store. + + An empty history presents exactly like a real answer, so this failure has + to be loud. Pinned because Layer 1 depends on the override, and a silent + mint would make the harness look like it isolated when it had actually + invented a new store. + """ + monkeypatch.setenv("SYNAPT_RECALL_ROOT", str(tmp_path / "typo-never-created")) + core_mod._gripspace_cache.clear() + + with pytest.raises(ValueError, match="does not exist"): + core_mod.project_data_dir() diff --git a/tests/recall_store_isolation.py b/tests/recall_store_isolation.py new file mode 100644 index 00000000..747ee3b7 --- /dev/null +++ b/tests/recall_store_isolation.py @@ -0,0 +1,215 @@ +"""Test-only store-isolation policy for the recall suite. + +Ref #955 — closes at promotion. + +This module holds the policy itself so that both the root ``conftest.py`` and +any nested pytest run (the mark/option witnesses use ``pytester``) can install +the *same* object rather than a lookalike. A witness that exercises a +reimplementation of the guard proves the reimplementation. + +Nothing here is imported by production code. The production side owns only a +pair of no-op seams that call whatever policy is installed; with no policy +installed they do nothing at all. +""" + +from __future__ import annotations + +import os +import pwd +from pathlib import Path + + +class RecallStoreIsolationError(AssertionError): + """Raised when a test would resolve a write target into a protected store. + + Deliberately an ``AssertionError`` subclass: this is a test-suite contract + violation, not a runtime error in the code under test, and it must fail the + test rather than be swallowed by an ``except Exception`` in a fixture. + """ + + +def account_home() -> Path: + """The home directory of the operating-system account. + + Read from the passwd database rather than ``Path.home()`` or ``$HOME`` + precisely because a fixture can move both of those. The protected boundary + must not be derivable from the value a test is currently asking the code to + use, or every guarantee here becomes circular. + """ + return Path(pwd.getpwuid(os.getuid()).pw_dir).resolve() + + +def protected_channel_root() -> Path: + """The real home-level channel store — the thing being protected.""" + return account_home() / ".synapt" / "channels" + + +def _normalise(path: Path) -> Path: + """Resolve symlinks and ``..`` without requiring the path to exist. + + Containment is a path property, not a string property. A candidate that + merely *looks* outside the store while resolving inside it is the case a + ``startswith`` check waves through. + """ + try: + return Path(path).expanduser().resolve() + except (OSError, RuntimeError): + return Path(os.path.normpath(os.path.abspath(str(path)))) + + +def contained_by(candidate: Path, root: Path) -> bool: + """True when *candidate* is *root* itself or lives beneath it. + + ``~/.synapt/channels-backup`` is not beneath ``~/.synapt/channels`` even + though its text starts with it. The boundary is closed at the root itself, + so the root is refused too rather than only its children. + """ + c = _normalise(candidate) + r = _normalise(root) + return c == r or r in c.parents + + +class StoreIsolationPolicy: + """Decides whether a resolved store path may be created or opened. + + Two protected sets, deliberately asymmetric: + + * ``_permanent`` always contains the real home store and can never be + removed. A test may *add* a decoy root to witness refusal mechanics + without ever being able to *remove* the real one — so the witnesses can + prove prevention without any of them attempting a write at the real + store. + * ``_extra`` holds those per-test decoys and is unwound after each test. + """ + + def __init__(self) -> None: + self._permanent: list[Path] = [protected_channel_root()] + self._extra: list[Path] = [] + self.session_root: Path | None = None + self.session_channel_root: Path | None = None + self.session_data_root: Path | None = None + self.allow_live: bool = False + self.current_item = None + self._announced: set[str] = set() + + # -- protected-root bookkeeping --------------------------------------- + + def roots(self) -> list[Path]: + return [*self._permanent, *self._extra] + + def register_extra_root(self, root: Path) -> None: + self._extra.append(Path(root)) + + def clear_extra_roots(self) -> None: + self._extra.clear() + + # -- the decision ------------------------------------------------------ + + def _nodeid(self) -> str: + item = self.current_item + return getattr(item, "nodeid", "") if item is not None else "" + + def _is_marked_live(self) -> bool: + item = self.current_item + if item is None: + return False + return item.get_closest_marker("live_channel_store") is not None + + def check_channel_path(self, operation: str, path: Path) -> None: + """Refuse a channel-store write target that lands in a protected root.""" + for root in self.roots(): + if contained_by(path, root): + if self._is_marked_live() and self.allow_live: + self._announce(root, path) + return + raise RecallStoreIsolationError( + self._message(operation, path, root) + ) + + def check_data_root(self, operation: str, path: Path) -> None: + """Require an implicit recall data root to be pytest-owned. + + Opposite polarity to the channel check: channels are refused when they + land somewhere named, data roots are refused unless they land somewhere + owned. A test that avoids the channel guard entirely can still read the + operator's real journal and make an assertion depend on history that + is not in the repository. + """ + if self.session_data_root is None: + return + if contained_by(path, self.session_data_root): + return + if contained_by(path, _tmp_root()): + return + raise RecallStoreIsolationError( + "recall test isolation violation\n" + f"nodeid={self._nodeid()}\n" + f"operation={operation}\n" + f"resolved={_normalise(path)}\n" + f"expected under={self.session_data_root}\n" + "an implicit recall data path escaped the pytest-owned root; pass a " + "tmp_path-based project_dir, or set SYNAPT_RECALL_ROOT to a " + "pytest-owned directory" + ) + + # -- reporting --------------------------------------------------------- + + def _message(self, operation: str, path: Path, root: Path) -> str: + """The refusal text. + + Names the resolved candidate and the test, because the failure is + usually read by someone who did not write the fixture that caused it. + The remedy offered is isolation. The opt-in is deliberately absent: a + message that leads with the bypass turns the guard into a documented + workaround, and the next reader reaches for the flag instead of fixing + the fixture. + """ + return ( + "recall test isolation violation\n" + f"nodeid={self._nodeid()}\n" + f"operation={operation}\n" + f"resolved={_normalise(path)}\n" + f"protected={_normalise(root)}\n" + "set SYNAPT_SHARED_CHANNELS_DIR to a pytest-owned directory, or " + "pass channels_dir=tmp_path/'channels'" + ) + + def _announce(self, root: Path, path: Path) -> None: + """Say plainly that a test was let through to a real store. + + The opt-in path stays loud so a reviewer can tell an authorized + integration test from the ordinary isolation contract. + """ + key = f"{self._nodeid()}:{_normalise(path)}" + if key in self._announced: + return + self._announced.add(key) + text = ( + f"[recall] {self._nodeid()} is authorized to touch the live " + f"channel store at {_normalise(root)} " + f"(--allow-live-channel-store-tests + live_channel_store mark)" + ) + # Written through the terminal reporter rather than print(): pytest + # captures stdout and only replays it for FAILING tests, so a printed + # announcement is invisible in exactly the case it exists for — an + # authorized write that succeeds. "Loud" has to mean loud on success. + item = self.current_item + if item is not None: + reporter = item.config.pluginmanager.get_plugin("terminalreporter") + if reporter is not None: + reporter.write_line(text) + return + print(text) + + +def _tmp_root() -> Path: + """The platform temporary directory. + + ``tmp_path`` lives beneath this, so an explicit ``project_dir=tmp_path`` + stays allowed without each test having to opt in. It is a safe allowance + because no real store lives here — the risk this guard exists for is a + resolved path escaping *into a checkout*, not into a scratch directory. + """ + import tempfile + + return _normalise(Path(tempfile.gettempdir())) From b9558e05741bfee888db242dc8e9d7b2b2d3fbf5 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Fri, 7 Aug 2026 12:55:50 -0500 Subject: [PATCH 02/19] test(recall): guard the state-db surface, re-arm the policy per test Review response on the store-isolation guard. Ref #955 -- closes at promotion. r2 found the seam covered the JSONL surface and not the SQLite one, proven with a recording policy: channel_post consulted it, the SQLite paths did not. The finding was right; the two SQLite paths turn out to need opposite treatment, so each gets the one the evidence assigns it. _open_state_db IS guarded. It takes its path from the caller, exactly the shape already recognised for an explicit channels_dir, so resolution never sees it -- and unlike the per-gripspace database it can land inside the global store. A probe put a caller-supplied path there with the policy consulted zero times before the file was created. Now refused, with a witness that asserts the file does not exist afterwards. _db_path is deliberately NOT guarded by the channel policy, and the docstring now says so. It is composed from project_data_dir, so it cannot resolve inside the global store: probed directly, and probed again with the data root forced at the parent of the protected root, and it stayed outside both times. A channel-store check there could not refuse anything. Adding one would read as tightening coverage while installing a check that cannot fail, which is the defect this seam exists to prevent. Its real coverage is the data-root policy, which project_data_dir consults -- verified by probe, not assumed. A witness pins both halves so the asymmetry is not "fixed" later by someone reading the seam list and seeing an oversight. Separately, r2 showed importlib.reload silently clears both policies: armed before, disarmed after, no error. That cannot be fixed where it happens, because a reload replaces the module wholesale. Patching the two known mechanisms is the weaker move -- the next one arrives unannounced with the same signature, a green suite and absent protection. So the policy is re-armed before every test instead, which closes the class rather than the instances. Cost is one identity check against None. Whole suite at base 1245f62: 23 failed / 2795 passed, against a 23 / 2768 baseline. Net new failures zero; the +27 passed is exactly the 27 witnesses. Guarding the state-db surface surfaced no further true positives. Premium boundary: recall is OSS -- test infrastructure plus the same neutral, no-op policy seam. No identity, no org semantics, no entitlement behaviour. --- conftest.py | 48 ++++++++++++++ src/synapt/recall/channel.py | 13 ++++ tests/recall/test_store_isolation_guard.py | 77 ++++++++++++++++++++++ 3 files changed, 138 insertions(+) diff --git a/conftest.py b/conftest.py index 5f9c581e..95e97dee 100644 --- a/conftest.py +++ b/conftest.py @@ -185,6 +185,41 @@ def pytest_unconfigure(config): def pytest_runtest_setup(item): POLICY.current_item = item + _rearm_if_disarmed(item.config) + + +def _rearm_if_disarmed(config): + """Re-install the policy before each test if anything cleared it. + + The guard lives in a module global, so ANY mechanism that replaces or + resets that module silently disarms it — and a disarmed guard is invisible: + the suite stays green while the protection it advertises is gone. Two such + mechanisms are known. A nested in-process pytest run whose unconfigure + cleared the global (fixed at its source with save-and-restore), and + ``importlib.reload``, which rebinds the module dict wholesale and cannot be + fixed at the source at all. + + Enumerating those two and patching each is the weaker move; the next + mechanism arrives unannounced with the same silent signature. Re-arming per + test closes over the whole class instead — whatever cleared it, the next + test starts armed. Cost is one identity check against None. + + SCOPE, stated rather than left to inference: this closes CROSS-test + leakage, not intra-test. A disarm that happens *during* a test leaves the + guard off for the remainder of THAT test; the next one starts armed. So the + blast radius drops from "every test that follows, indefinitely" to "the + rest of this one," which is the right trade at this cost — but it is a + reduction, not an elimination. Saying "the class is closed" without this + sentence would invite exactly the inference this whole harness exists to + prevent: reading a partial guarantee as a total one. + """ + from synapt.recall import channel as channel_mod + from synapt.recall import core as core_mod + + if channel_mod._store_path_policy is None: + channel_mod.set_store_path_policy(POLICY.check_channel_path) + if config.getoption("--strict-recall-data-root") and core_mod._data_root_policy is None: + core_mod.set_data_root_policy(POLICY.check_data_root) def pytest_runtest_call(item): @@ -259,6 +294,19 @@ def _register(root: Path) -> Path: return _register +@pytest.fixture +def rearm_guard(request): + """The per-test re-arm routine, for the witness that a reload cannot disarm it. + + Exposed as a fixture rather than imported: ``import conftest`` resolves to + the nearest conftest on sys.path, which is not this one. + """ + def _rearm(): + _rearm_if_disarmed(request.config) + + return _rearm + + @pytest.fixture def isolation_policy(): """The live policy object, for witnesses that inspect the harness itself.""" diff --git a/src/synapt/recall/channel.py b/src/synapt/recall/channel.py index c2101eb5..63575031 100644 --- a/src/synapt/recall/channel.py +++ b/src/synapt/recall/channel.py @@ -272,6 +272,15 @@ def _db_path(project_dir: Path | None = None) -> Path: Always uses the local directory — presence, cursors, pins, and mutes are per-gripspace even when channels are shared. + + Deliberately NOT guarded by the channel-store policy, and the reason is + load-bearing rather than an oversight. This path is composed from + ``project_data_dir``, so it can never resolve inside the global channel + store; a channel-store check here could not refuse anything, and a check + that cannot fail is the defect this seam exists to prevent. It is instead + covered by the data-root policy, which ``project_data_dir`` consults — + verified by probe, not assumed. See ``_open_state_db`` for the SQLite path + that CAN reach the global store and is guarded accordingly. """ return _local_channels_dir(project_dir) / "channels.db" @@ -3235,6 +3244,10 @@ def _migrate_cursors( def _open_state_db(state_db: Path) -> sqlite3.Connection: """Open or create the global _state.db with WAL mode.""" + # Caller-supplied path, same shape as an explicit channels_dir: it reaches + # around resolution entirely, so it is guarded in its own right. This one + # can land inside the global store, unlike the per-gripspace channels.db. + _guard_store_path("open_state_db", state_db) state_db.parent.mkdir(parents=True, exist_ok=True) conn = sqlite3.connect(str(state_db)) conn.row_factory = sqlite3.Row diff --git a/tests/recall/test_store_isolation_guard.py b/tests/recall/test_store_isolation_guard.py index 28571127..71ac0175 100644 --- a/tests/recall/test_store_isolation_guard.py +++ b/tests/recall/test_store_isolation_guard.py @@ -354,6 +354,83 @@ def test_local_to_global_migration_is_covered( assert _entries(decoy_root) == before +# --------------------------------------------------------------------------- +# The SQLite surfaces — and why exactly one of them is guarded +# --------------------------------------------------------------------------- + +def test_state_db_at_the_store_is_refused_before_the_file_is_created( + register_protected_root, store_isolation_error, tmp_path +): + """``_open_state_db`` takes its path from the caller and can reach the store. + + Same shape as an explicit ``channels_dir``: it reaches around resolution + entirely, so the resolver guard cannot see it. Found in review — the JSONL + surface was covered and this one was not, and nothing said so. + """ + decoy_root = tmp_path / "home" / ".synapt" / "channels" + register_protected_root(decoy_root) + + target = decoy_root / "decoy-org" / "decoy-repo" / "_state.db" + before = _entries(decoy_root) + + with pytest.raises(store_isolation_error): + channel_mod._open_state_db(target) + + assert _entries(decoy_root) == before + assert not target.exists() + + +def test_channels_db_is_data_root_surface_not_channel_store_surface( + protected_channel_root, tmp_path +): + """``channels.db`` is deliberately outside the channel guard, and must stay so. + + It is composed from ``project_data_dir``, so it can never resolve inside the + global channel store — meaning a channel-store check here could not refuse + anything. Adding one would look like tightening coverage while installing a + check that cannot fail, which is the exact defect this harness exists to + prevent. + + Pinned because the asymmetry looks like an oversight to anyone reading the + seam list, and the obvious "fix" is the wrong move. The real coverage comes + from the data-root policy, which ``project_data_dir`` consults. + """ + resolved = channel_mod._db_path(project_dir=tmp_path).resolve() + + assert protected_channel_root not in resolved.parents + assert resolved != protected_channel_root + + consulted = [] + previous = core_mod.set_data_root_policy(lambda op, p: consulted.append(op)) + try: + channel_mod._db_path(project_dir=tmp_path) + finally: + core_mod.set_data_root_policy(previous) + + assert consulted, "channels.db must be reachable by the data-root policy" + + +# --------------------------------------------------------------------------- +# The guard must survive anything that clears the module global +# --------------------------------------------------------------------------- + +def test_guard_is_rearmed_after_a_module_reload_disarms_it(rearm_guard): + """``importlib.reload`` rebinds the module dict and silently clears the policy. + + It cannot be fixed where it happens — a reload replaces the module wholesale + — so the harness re-arms before each test instead. That closes the whole + class rather than the two mechanisms currently known, because the next one + will arrive with the same signature: green suite, absent protection. + """ + import importlib + + importlib.reload(channel_mod) + assert channel_mod._store_path_policy is None, "reload should clear the policy" + + rearm_guard() + assert channel_mod._store_path_policy is not None + + # --------------------------------------------------------------------------- # W6 / W7 — containment is a path property, not a string property # --------------------------------------------------------------------------- From 7e1f0750a6b320ad8b6cd092d48b351fe10c5448 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Fri, 7 Aug 2026 13:52:14 -0500 Subject: [PATCH 03/19] test(recall): isolate the 30 data-root sites so the strict flag can be armed Ref #967 -- closes at promotion. Phase 1 of the burn-down. The data-root half of the store-isolation guard shipped behind --strict-recall-data-root, off by default, because arming it failed 30 tests across 10 files. Those were true positives: tests/recall deliberately strips the root override so tests measure path INFERENCE, and that inference resolved into a real checkout. Every one of the 30 read or wrote the operator's live .synapt/recall. The count was re-derived at this base before any edit rather than carried forward from the issue. It came back 30 with an identical per-file distribution, but the check was not ceremonial: the original figure was taken before the guard merged, so it was measured under a configuration that no longer existed. Isolation preserves the fixture's intent instead of overriding it. The autouse fixture strips AMBIENT overrides so these tests measure inference, and that is correct; its own docstring already carved out the other half -- "unless a test sets the override itself." So two mechanisms, chosen per site by what the test is actually asserting: A test that NEEDS a store gets an owned root, via a new owned_recall_root fixture for pytest-style tests and an owned_store helper for the unittest.TestCase classes that cannot take fixtures. One helper rather than a hand-rolled save/restore per class, because five copies of an environment restore is five chances to restore the wrong thing. A test that is genuinely measuring inference keeps measuring it, and moves its CWD to a directory it owns instead. recall_save hardcodes project = Path.cwd(), so it has no project_dir to pass and the environment override does not apply to an explicit path; redirecting the root there would have looked like isolation while changing nothing. One site was a real bug rather than missing isolation: two lines called the live _reminders_path() and mkdir'd its parent inside the operator's store, then discarded the value, because the patch immediately below supplies the path actually used. Dead code that wrote to a real location while reading as setup for a temporary one. Whole suite on one runner at this base: before, default 23 failed / 2795 passed before, strict 53 failed / 2765 passed after, default 23 failed / 2795 passed after, strict 23 failed / 2795 passed The burn-down set -- strict minus default -- goes from 30 to 0, and the two modes now produce identical results, which is the property the flip depends on. Zero new failures against the default baseline. The 23 are pre-existing at this base and untouched here. Flipping the default is deliberately NOT in this change. The flag stays off so this can be reviewed as isolation work on its own evidence; the flip is a separate one-line change that now has nothing left to hide behind it. Premium boundary: recall is OSS -- test isolation only. No production code is touched by this commit. No identity, no org semantics, no entitlement behaviour. --- tests/recall/_isolation_helpers.py | 62 ++++++++++++++++++++++++++++ tests/recall/conftest.py | 27 ++++++++++++ tests/recall/test_action_registry.py | 32 +++----------- tests/recall/test_channel_hooks.py | 17 ++++---- tests/recall/test_clustering.py | 9 +++- tests/recall/test_codex.py | 22 ++++++++++ tests/recall/test_dm_channels.py | 32 ++++++++++++++ tests/recall/test_freshness.py | 6 +-- tests/recall/test_live.py | 12 ++++++ tests/recall/test_resume.py | 8 ++++ tests/recall/test_startup.py | 11 ++--- tests/recall/test_supersession.py | 21 +++++++--- 12 files changed, 210 insertions(+), 49 deletions(-) create mode 100644 tests/recall/_isolation_helpers.py diff --git a/tests/recall/_isolation_helpers.py b/tests/recall/_isolation_helpers.py new file mode 100644 index 00000000..e021b0d9 --- /dev/null +++ b/tests/recall/_isolation_helpers.py @@ -0,0 +1,62 @@ +"""Store-isolation helpers for ``unittest.TestCase`` recall tests. + +Ref #967. + +Pytest-style tests use the ``owned_recall_root`` fixture in ``conftest.py``. +``unittest.TestCase`` methods cannot receive fixtures, so those classes reach +for this instead — one helper rather than a hand-rolled setUp/tearDown pair per +class, because five copies of an environment save/restore is five chances to +restore the wrong thing. + +The distinction the fixture docstring draws applies here identically: this is +for a test that NEEDS a store, not for one measuring where a store is inferred +from. A test genuinely asserting inference should chdir to an owned directory +so the inference still runs, just from a starting point it owns. +""" + +from __future__ import annotations + +import os +import tempfile +from pathlib import Path + +_VARS = ("SYNAPT_RECALL_ROOT", "SYNAPT_RECALL_WORKTREE", "SYNAPT_SHARED_CHANNELS_DIR") + + +class OwnedStore: + """A restorable set of store overrides pointing at a throwaway directory. + + Restores the PREVIOUS value rather than deleting, because these tests run + under an autouse fixture that has already removed any ambient override — + deleting on teardown would work today and silently diverge from that + fixture's intent the moment it changes. + """ + + def __init__(self, root: Path, previous: dict[str, str | None]) -> None: + self.root = root + self._previous = previous + + def restore(self) -> None: + for name, prev in self._previous.items(): + if prev is None: + os.environ.pop(name, None) + else: + os.environ[name] = prev + + +def owned_store() -> OwnedStore: + """Point recall data and channel resolution at a directory this test owns. + + The data root is created before it is pointed at: the override refuses a + root that does not exist, on the grounds that silently minting a fresh + store under a mistyped path presents an empty history as a real answer. + """ + base = Path(tempfile.mkdtemp()) + data_root = base / "recall-root" + data_root.mkdir(parents=True, exist_ok=True) + + previous = {name: os.environ.get(name) for name in _VARS} + os.environ["SYNAPT_RECALL_ROOT"] = str(data_root) + os.environ["SYNAPT_RECALL_WORKTREE"] = "pytest-owned" + os.environ["SYNAPT_SHARED_CHANNELS_DIR"] = str(base / "channels") + return OwnedStore(base, previous) diff --git a/tests/recall/conftest.py b/tests/recall/conftest.py index 57c1981f..c3d92fc9 100644 --- a/tests/recall/conftest.py +++ b/tests/recall/conftest.py @@ -215,3 +215,30 @@ def _isolate_recall_root_env(monkeypatch): """ monkeypatch.delenv("SYNAPT_RECALL_ROOT", raising=False) monkeypatch.delenv("SYNAPT_RECALL_WORKTREE", raising=False) + + +@pytest.fixture +def owned_recall_root(tmp_path, monkeypatch) -> Path: + """Point implicit recall data resolution at a directory this test owns. + + Ref #967. The autouse fixture above strips an *ambient* override so these + tests measure inference rather than a value the shell happened to set — + that intent is right and is preserved. This fixture is the other half it + already anticipated: "unless a test sets the override itself." Requesting + it is a test declaring that it needs a store, not that it is measuring + where one is inferred from. + + Use this when the test needs recall data to exist somewhere. When the test + is genuinely asserting *inference* behaviour, prefer ``monkeypatch.chdir`` + to an owned directory instead, so the inference still runs — just from a + starting point the test owns rather than from the operator's checkout. + + The directory is created before it is handed over, because the override + refuses a root that does not exist: silently minting a fresh store under a + mistyped path would present an empty history as a real answer. + """ + root = tmp_path / "recall-root" + root.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv("SYNAPT_RECALL_ROOT", str(root)) + monkeypatch.setenv("SYNAPT_RECALL_WORKTREE", "pytest-owned") + return root diff --git a/tests/recall/test_action_registry.py b/tests/recall/test_action_registry.py index 7f4a776a..98fa7248 100644 --- a/tests/recall/test_action_registry.py +++ b/tests/recall/test_action_registry.py @@ -7,34 +7,12 @@ All tests are expected to FAIL until the implementation lands. """ -import os -import tempfile import unittest -from dataclasses import dataclass -from pathlib import Path from unittest.mock import MagicMock, patch +from _isolation_helpers import owned_store -@dataclass -class _ChannelRootPatch: - """A restorable ``SYNAPT_SHARED_CHANNELS_DIR`` override.""" - previous: str | None - - def restore(self) -> None: - if self.previous is None: - os.environ.pop("SYNAPT_SHARED_CHANNELS_DIR", None) - else: - os.environ["SYNAPT_SHARED_CHANNELS_DIR"] = self.previous - - -def _isolated_channel_root() -> _ChannelRootPatch: - """Point channel resolution at a throwaway directory for one test.""" - previous = os.environ.get("SYNAPT_SHARED_CHANNELS_DIR") - os.environ["SYNAPT_SHARED_CHANNELS_DIR"] = str( - Path(tempfile.mkdtemp()) / "channels" - ) - return _ChannelRootPatch(previous) # OSS base actions that must always be available @@ -76,10 +54,10 @@ class TestOSSBaseActions(unittest.TestCase): """OSS base actions must be registered by default.""" def setUp(self): - self._channels = _isolated_channel_root() + self._store = owned_store() def tearDown(self): - self._channels.restore() + self._store.restore() def test_oss_actions_registered(self): """All OSS base actions should be in the default registry.""" @@ -294,13 +272,13 @@ def setUp(self): # These dispatch through the real channel post path. With no override # they resolve to the home-level store and their fixture messages land # in live channels (Ref #955). - self._channels = _isolated_channel_root() + self._store = owned_store() def tearDown(self): from synapt.recall.actions import reset_action_registry reset_action_registry() - self._channels.restore() + self._store.restore() def test_recall_channel_uses_registry_dispatch(self): """recall_channel should route OSS actions through the shared registry.""" diff --git a/tests/recall/test_channel_hooks.py b/tests/recall/test_channel_hooks.py index fb6c3fb9..3bbda383 100644 --- a/tests/recall/test_channel_hooks.py +++ b/tests/recall/test_channel_hooks.py @@ -8,10 +8,10 @@ - Hooks are best-effort (exceptions don't break posting) """ -import os -import tempfile import unittest +from _isolation_helpers import owned_store + from synapt.recall.channel import ( ChannelMessage, _append_message, @@ -27,10 +27,13 @@ class TestMessageHooks(unittest.TestCase): """Tests for the message hook registration and dispatch.""" def setUp(self): - self._tmp = tempfile.mkdtemp() - # SYNAPT_DATA_DIR is read nowhere in the package; it isolated nothing and - # these tests appended into the real channel store (Ref #955). - os.environ["SYNAPT_SHARED_CHANNELS_DIR"] = self._tmp + # SYNAPT_DATA_DIR was read nowhere in the package; it isolated nothing + # and these tests appended into the real channel store (Ref #955). The + # channel override alone still left the recall DATA root inferred from + # cwd, which lands in a real checkout (Ref #967) — owned_store covers + # both surfaces and restores previous values rather than deleting. + self._store = owned_store() + self._tmp = str(self._store.root) # Save original hooks self._original_hooks = list(_message_posted_hooks) @@ -39,7 +42,7 @@ def tearDown(self): _clear_message_hooks() for hook in self._original_hooks: register_message_hook(hook) - os.environ.pop("SYNAPT_SHARED_CHANNELS_DIR", None) + self._store.restore() def test_default_hooks_registered(self): """Default mention and wake hooks should be registered at import time.""" diff --git a/tests/recall/test_clustering.py b/tests/recall/test_clustering.py index 8e5f6d21..4e513069 100644 --- a/tests/recall/test_clustering.py +++ b/tests/recall/test_clustering.py @@ -816,8 +816,13 @@ def test_cluster_drilldown(self, tmp_path): assert "1 chunks" in result db.close() - def test_nonexistent_cluster(self, tmp_path): - """recall_context with nonexistent cluster_id returns helpful message.""" + def test_nonexistent_cluster(self, owned_recall_root, tmp_path): + """recall_context with nonexistent cluster_id returns helpful message. + + Ref #967 — ``recall_context`` resolves the data root implicitly, so + without an owned root the "not found" could come from the operator's + store rather than from the empty index this test builds. + """ from unittest.mock import patch from synapt.recall.storage import RecallDB from synapt.recall.core import TranscriptIndex diff --git a/tests/recall/test_codex.py b/tests/recall/test_codex.py index cc73e0c9..452a96dc 100644 --- a/tests/recall/test_codex.py +++ b/tests/recall/test_codex.py @@ -1,6 +1,7 @@ """Tests for synapt.recall.codex — Codex CLI transcript parsing.""" import json +import os import tempfile import unittest from pathlib import Path @@ -572,6 +573,27 @@ class TestTheBuildPreCheckReadsTheCodexArm(unittest.TestCase): pre-check branch rather than poking the helper. """ + def setUp(self): + # cmd_build resolves the recall data root implicitly, so without an + # owned root this drives the pre-check against the operator's live + # store (Ref #967). unittest.TestCase cannot take the shared fixture, + # so the override is set here and restored in tearDown. + self._tmp = tempfile.mkdtemp() + self._prev = os.environ.get("SYNAPT_RECALL_ROOT") + self._prev_wt = os.environ.get("SYNAPT_RECALL_WORKTREE") + os.environ["SYNAPT_RECALL_ROOT"] = self._tmp + os.environ["SYNAPT_RECALL_WORKTREE"] = "pytest-owned" + + def tearDown(self): + for name, prev in ( + ("SYNAPT_RECALL_ROOT", self._prev), + ("SYNAPT_RECALL_WORKTREE", self._prev_wt), + ): + if prev is None: + os.environ.pop(name, None) + else: + os.environ[name] = prev + def _run_precheck(self, has_codex: bool): """Drive cmd_build's pre-check with everything downstream stubbed.""" import argparse diff --git a/tests/recall/test_dm_channels.py b/tests/recall/test_dm_channels.py index da3de230..e9d9722c 100644 --- a/tests/recall/test_dm_channels.py +++ b/tests/recall/test_dm_channels.py @@ -56,8 +56,16 @@ def setUp(self): # nothing — these tests resolved to the real home-level channel store. # SYNAPT_SHARED_CHANNELS_DIR is the override the resolver actually # consults (Ref #955). + # Ref #967: the channel override alone leaves the recall DATA root + # inferred from cwd, which lands in a real checkout. The data root must + # exist before it is pointed at — the override refuses a missing root + # rather than mint an empty store that reads like a real answer. + recall_root = Path(self.tmpdir) / "recall-root" + recall_root.mkdir(parents=True, exist_ok=True) self._env_patch = patch.dict(os.environ, { "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), + "SYNAPT_RECALL_ROOT": str(recall_root), + "SYNAPT_RECALL_WORKTREE": "pytest-owned", }) self._env_patch.start() @@ -117,8 +125,16 @@ def setUp(self): # nothing — these tests resolved to the real home-level channel store. # SYNAPT_SHARED_CHANNELS_DIR is the override the resolver actually # consults (Ref #955). + # Ref #967: the channel override alone leaves the recall DATA root + # inferred from cwd, which lands in a real checkout. The data root must + # exist before it is pointed at — the override refuses a missing root + # rather than mint an empty store that reads like a real answer. + recall_root = Path(self.tmpdir) / "recall-root" + recall_root.mkdir(parents=True, exist_ok=True) self._env_patch = patch.dict(os.environ, { "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), + "SYNAPT_RECALL_ROOT": str(recall_root), + "SYNAPT_RECALL_WORKTREE": "pytest-owned", }) self._env_patch.start() @@ -186,8 +202,16 @@ def setUp(self): # nothing — these tests resolved to the real home-level channel store. # SYNAPT_SHARED_CHANNELS_DIR is the override the resolver actually # consults (Ref #955). + # Ref #967: the channel override alone leaves the recall DATA root + # inferred from cwd, which lands in a real checkout. The data root must + # exist before it is pointed at — the override refuses a missing root + # rather than mint an empty store that reads like a real answer. + recall_root = Path(self.tmpdir) / "recall-root" + recall_root.mkdir(parents=True, exist_ok=True) self._env_patch = patch.dict(os.environ, { "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), + "SYNAPT_RECALL_ROOT": str(recall_root), + "SYNAPT_RECALL_WORKTREE": "pytest-owned", }) self._env_patch.start() @@ -261,8 +285,16 @@ def setUp(self): # nothing — these tests resolved to the real home-level channel store. # SYNAPT_SHARED_CHANNELS_DIR is the override the resolver actually # consults (Ref #955). + # Ref #967: the channel override alone leaves the recall DATA root + # inferred from cwd, which lands in a real checkout. The data root must + # exist before it is pointed at — the override refuses a missing root + # rather than mint an empty store that reads like a real answer. + recall_root = Path(self.tmpdir) / "recall-root" + recall_root.mkdir(parents=True, exist_ok=True) self._env_patch = patch.dict(os.environ, { "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), + "SYNAPT_RECALL_ROOT": str(recall_root), + "SYNAPT_RECALL_WORKTREE": "pytest-owned", }) self._env_patch.start() diff --git a/tests/recall/test_freshness.py b/tests/recall/test_freshness.py index 9832c839..0cb2c122 100644 --- a/tests/recall/test_freshness.py +++ b/tests/recall/test_freshness.py @@ -396,7 +396,7 @@ def _set_manifest(project: Path, source_files: list[dict]) -> None: con.close() -def test_cmd_resume_prints_the_stale_banner(store): +def test_cmd_resume_prints_the_stale_banner(owned_recall_root, store): """The wiring, end to end: a stale store must say so in stdout.""" _real_index(store) _archive_file(_archive_dir(store), "rollout-unindexed.jsonl", "turns nobody indexed") @@ -408,7 +408,7 @@ def test_cmd_resume_prints_the_stale_banner(store): assert "recall build" in out, "the banner must carry its remedy" -def test_cmd_resume_stays_quiet_when_the_index_is_current(store): +def test_cmd_resume_stays_quiet_when_the_index_is_current(owned_recall_root, store): """Control: the banner is conditional, not unconditional decoration.""" _real_index(store) f = _archive_file(_archive_dir(store), "rollout-indexed.jsonl", "turns") @@ -420,7 +420,7 @@ def test_cmd_resume_stays_quiet_when_the_index_is_current(store): assert "a real question" in out, "the turns must still render" -def test_cmd_resume_runs_the_deep_leg_only_when_cheap_is_fresh_and_view_empty(store, monkeypatch): +def test_cmd_resume_runs_the_deep_leg_only_when_cheap_is_fresh_and_view_empty(owned_recall_root, store, monkeypatch): """The deep-trigger path, pinned at the call site rather than described.""" import synapt.recall.freshness as fresh_mod diff --git a/tests/recall/test_live.py b/tests/recall/test_live.py index 9af8c9cd..570911e0 100644 --- a/tests/recall/test_live.py +++ b/tests/recall/test_live.py @@ -7,6 +7,8 @@ import tempfile import threading import unittest + +from _isolation_helpers import owned_store from pathlib import Path from unittest.mock import MagicMock, patch @@ -494,6 +496,16 @@ def test_rewrites_file_in_chronological_order(self): class TestRecallSearchLiveIntegration(unittest.TestCase): + def setUp(self): + # Ref #967: recall_search resolves the recall data root implicitly, so + # the "unavailable" and "setup" messages these tests assert on could + # otherwise be produced against the operator's live store rather than + # against the absence this test is constructing. + self._store = owned_store() + + def tearDown(self): + self._store.restore() + def test_recall_search_combines_live_and_indexed(self): """recall_search should join 'Current session context:' and indexed results.""" from synapt.recall.server import recall_search diff --git a/tests/recall/test_resume.py b/tests/recall/test_resume.py index 1478a2c9..54ac3d92 100644 --- a/tests/recall/test_resume.py +++ b/tests/recall/test_resume.py @@ -26,6 +26,8 @@ import json import tempfile import unittest + +from _isolation_helpers import owned_store from argparse import Namespace from pathlib import Path from unittest import mock @@ -691,8 +693,14 @@ class TestEmptyAndErrorStates(unittest.TestCase): def setUp(self): self.tmp = tempfile.TemporaryDirectory() self.dir = Path(self.tmp.name) + # Ref #967: the CLI path under test resolves the recall data root + # implicitly, so without an owned store these ran against a real + # checkout and their "empty" states could come from the operator's + # history rather than from the fixtures built here. + self._store = owned_store() def tearDown(self): + self._store.restore() self.tmp.cleanup() def test_missing_index_exits_nonzero_and_says_how_to_fix(self): diff --git a/tests/recall/test_startup.py b/tests/recall/test_startup.py index 4298338b..07f5bca3 100644 --- a/tests/recall/test_startup.py +++ b/tests/recall/test_startup.py @@ -72,12 +72,13 @@ def test_journal_entries_surfaced(self, tmp_path): def test_reminders_surfaced(self, tmp_path): """Pending reminders appear in startup context.""" - from synapt.recall.reminders import add_reminder, _reminders_path - - # Point reminders to tmp dir - rpath = _reminders_path() - rpath.parent.mkdir(parents=True, exist_ok=True) + from synapt.recall.reminders import add_reminder + # The two lines that used to stand here called the REAL _reminders_path() + # and mkdir'd its parent — creating a directory inside the operator's + # live store — and then threw the value away, because the patch below + # supplies the path that is actually used. Dead code that wrote to a + # real location while looking like setup for a temp one (Ref #967). with patch("synapt.recall.reminders._reminders_path") as mock_path: rfile = tmp_path / ".synapt" / "reminders.json" rfile.parent.mkdir(parents=True, exist_ok=True) diff --git a/tests/recall/test_supersession.py b/tests/recall/test_supersession.py index 02ade3a3..85e932a0 100644 --- a/tests/recall/test_supersession.py +++ b/tests/recall/test_supersession.py @@ -1220,7 +1220,7 @@ def _force_match(candidate, existing_nodes, threshold=0.80): assert candidate_id in historical_ids db.close() - def test_candidate_wins_promotes_candidate_supersedes_existing(self, tmp_path): + def test_candidate_wins_promotes_candidate_supersedes_existing(self, owned_recall_root, tmp_path): from synapt.recall.server import recall_contradict db, index, cid = self._make_contested_pair(tmp_path) @@ -1240,7 +1240,7 @@ def test_candidate_wins_promotes_candidate_supersedes_existing(self, tmp_path): # Queue entry resolved, no longer pending assert db.list_pending_contradictions() == [] - def test_existing_wins_restores_existing_retires_candidate(self, tmp_path): + def test_existing_wins_restores_existing_retires_candidate(self, owned_recall_root, tmp_path): from synapt.recall.server import recall_contradict db, index, cid = self._make_contested_pair(tmp_path) @@ -1259,7 +1259,7 @@ def test_existing_wins_restores_existing_retires_candidate(self, tmp_path): assert candidate["status"] == "stale" assert db.list_pending_contradictions() == [] - def test_false_positive_restores_both_supersedes_neither(self, tmp_path): + def test_false_positive_restores_both_supersedes_neither(self, owned_recall_root, tmp_path): from synapt.recall.server import recall_contradict db, index, cid = self._make_contested_pair(tmp_path) @@ -2364,9 +2364,17 @@ def test_recall_save_defaults_to_content_hash_upsert(self, tmp_path): finally: db.close() - def test_recall_save_requires_content(self): + def test_recall_save_requires_content(self, tmp_path, monkeypatch): + """Ref #967 — recall_save hardcodes ``project = Path.cwd()``. + + There is no project_dir to pass and the env override does not apply to + an explicit path, so the isolation here is to move the CWD rather than + redirect the root: the inference still runs, just from a directory this + test owns instead of the operator's checkout. + """ from synapt.recall.server import recall_save + monkeypatch.chdir(tmp_path) assert "required" in recall_save(content=" ").lower() def test_recall_save_upserts_stable_node_id(self, tmp_path): @@ -2457,9 +2465,12 @@ def test_recall_save_retract(self, tmp_path): finally: db.close() - def test_recall_save_retract_requires_node_id(self): + def test_recall_save_retract_requires_node_id(self, tmp_path, monkeypatch): + """Ref #967 — same cwd-derived resolution as the sibling above.""" from synapt.recall.server import recall_save + monkeypatch.chdir(tmp_path) + result = recall_save(retract=True) assert "node_id is required" in result.lower() From 429ac312493e15a0f4c1dec1fb6d8bb60375c582 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Fri, 7 Aug 2026 14:11:22 -0500 Subject: [PATCH 04/19] test(recall): arm the data-root guard by default Ref #967 -- closes at promotion. The 30 sites that resolved into a real checkout are isolated, so the guard no longer has anything to break. --strict-recall-data-root is now the default and --no-strict-recall-data-root is the debugging escape. THE EVIDENCE FOR THIS FLIP IS NOT THAT BOTH MODES AGREE. Before the burn-down, running with the flag differed from running without it, and that difference was the proof the flag was wired. Now they agree -- which is the success criterion and, identically, the signature of a guard that has stopped working. Modal agreement can no longer tell those apart, so citing it would be vacuous by construction. The evidence is two direct witnesses instead. One asserts the policy is installed with no flag passed and that an unowned path is still refused. The other asserts the escape hatch reaches the install decision, in two separately checkable links -- the option resolving to the destination the installer reads, and the installer declining to arm when told not to -- because either link alone is satisfiable without the other. The escape hatch also has to leave a GREEN suite or nobody will use it, so the armed-by-default witness skips with a stated reason under --no-strict-recall-data-root rather than failing. A debugging flag that reddens the suite is a flag people route around. Neither witness is a nested pytest run. pytester executes in-process and shares module globals, which is how this guard silently disarmed itself once already; a nested run to test disarming would aim that same hazard at the thing it broke before. Review also surfaced a third link nobody had pinned: when BOTH flags are passed, the last one wins. That was already the behaviour, but by argparse accident rather than by decision -- undocumented, untested, and free to change under a dependency bump with nothing going red. A CI base config carrying one flag and a job appending the other would have had its behaviour settled by argument order nobody wrote down. Last-wins is the conventional answer and a good one; a witness and one line of option help make it a decision instead of an accident. Two further links came from review, both about what the escape hatch is allowed to mean. The skip condition discriminates on the ARGV signal rather than the resolved option. Those agree today and come apart in exactly the case that matters: if the default is ever silently reverted, the resolved option reads off with no flag passed, and skipping there would announce "deliberately disarmed" about a disarm nobody requested. A false reason is worse than no reason, because it explains away the thing it should surface. It now fails and names the reverted default. And the hatch is SCOPED: disarming the data root must leave the channel guard armed. Nothing witnessed that, so the escape was free to widen into a full disarm -- taking a guard that was never opt-in -- while every other witness here still passed, because they arm their own policy or assert against a decoy. An escape hatch nobody has bounded is a hatch that grows. Both were mutation-checked rather than assumed: reverting the default makes the first fail, widening the hatch makes the second fail, and both restore byte-identical. The precedence witness runs in a SUBPROCESS rather than through pytester. The hazard that rules out nested in-process runs here is shared module globals -- a nested run can disarm the outer guard, and a green result would then be indistinguishable from the hazard firing. A subprocess shares no globals, so it is the one shape that can answer an argument-parsing question without risking the thing being asked about. Also consolidates test_codex onto the shared owned_store helper. It carried a hand-rolled two-variable save/restore -- the duplication that helper exists to remove -- because it was written before the helper existed. Whole suite: 23 failed / 2798 passed, against a 23 / 2795 baseline. The +3 are the witnesses this commit adds; the two review links are assertions inside existing witnesses rather than new ones. Zero new failures. Non-emptiness checked: 2797 is the full suite, not a vacuous green over an empty collection. Premium boundary: recall is OSS -- test configuration only. No production code is touched. No identity, no org semantics, no entitlement behaviour. --- conftest.py | 95 +++++++++++++++----- tests/recall/test_codex.py | 26 ++---- tests/recall/test_store_isolation_guard.py | 100 +++++++++++++++++++++ 3 files changed, 181 insertions(+), 40 deletions(-) diff --git a/conftest.py b/conftest.py index 95e97dee..efd0c7e6 100644 --- a/conftest.py +++ b/conftest.py @@ -74,13 +74,22 @@ def pytest_addoption(parser): parser.addoption( "--strict-recall-data-root", action="store_true", - default=False, + default=True, + help=( + "Accepted and now the default; kept so existing invocations do not " + "break. Implicit recall data paths (journal, index, archive, " + "knowledge) that resolve outside a pytest-owned root are refused." + ), + ) + parser.addoption( + "--no-strict-recall-data-root", + action="store_false", + dest="strict_recall_data_root", help=( - "Also refuse implicit recall data paths (journal, index, archive, " - "knowledge) that resolve outside a pytest-owned root. Off by " - "default: the suite currently has real sites that resolve into the " - "operator's live store, and arming this before they are isolated " - "would turn a true finding into a red suite." + "Disable the data-root guard for a debugging run. Not for CI: with " + "it off, a test that resolves into a real checkout passes silently, " + "which is the condition this guard exists to end. If both this and " + "--strict-recall-data-root are given, the LAST one wins." ), ) @@ -122,7 +131,7 @@ def pytest_configure(config): # store, which is another workspace's namespace rather than our own. os.environ.setdefault("SYNAPT_RECALL_WORKTREE", "pytest-isolated") - _install_policy(config.getoption("--strict-recall-data-root")) + _install_policy(config.getoption("strict_recall_data_root")) def _install_policy(strict_data_root: bool): @@ -132,23 +141,23 @@ def _install_policy(strict_data_root: bool): for. It refuses without redirecting, so it does not change where anything legitimately resolves. - The data-root guard is opt-in *for now*, and the reason is worth stating - plainly rather than hiding behind a default. Arming it fails 30 further - tests across 10 files, and those failures are TRUE POSITIVES: + The data-root guard is now ALSO on by default. It shipped opt-in because + arming it failed 30 tests across 10 files — true positives, since ``tests/recall`` deliberately strips the root override so tests measure - path inference, and that inference currently lands in a real checkout's - ``.synapt/recall``. Isolating those sites is real work and a separate - reviewable change. Shipping it armed would mean either a red suite or 30 - edits buried in a guard PR; shipping it absent would lose the mechanism. So - it ships built, witnessed, and one flag away — and the count is reported - rather than quietly carried. - - That count is 30 measured at base 1245f62, not the ~60 an earlier draft - carried. The larger figure was taken while this file still installed a - session-wide channel override, which was itself breaking 30 tests; half the - "true positives" were the harness's own doing. A measurement inherits the - configuration it was taken under, and a number quoted after the - configuration changed is a stale claim wearing a precise costume. + path inference, and that inference resolved into a real checkout. Those 30 + were isolated and the flag was flipped. + + ONE WARNING FOR WHOEVER TOUCHES THIS NEXT, because the obvious evidence is + the wrong evidence. Before the burn-down, running with the flag differed + from running without it, and THAT DIFFERENCE was the proof the flag was + wired. Now the two agree — which is the success criterion and, identically, + the signature of a guard that has stopped working. Modal agreement can no + longer distinguish them, so it must never be cited as evidence this guard + functions. The evidence is the direct witnesses in + ``tests/recall/test_store_isolation_guard.py``, which exercise the policy + itself rather than observing the flag. + + ``--no-strict-recall-data-root`` exists for debugging and is not for CI. """ from synapt.recall import channel as channel_mod from synapt.recall import core as core_mod @@ -307,6 +316,46 @@ def _rearm(): return _rearm +@pytest.fixture +def pytester_precedence(): + """Measure flag precedence in a SEPARATE PROCESS, both orderings. + + Deliberately a subprocess and not ``pytester``'s in-process runner. The + hazard that rules out nested in-process runs elsewhere in this file is + shared module globals — a nested run can disarm the outer guard, and a + green result would then be indistinguishable from the hazard firing. A + subprocess has no globals in common, so it is the one shape that can answer + an argument-parsing question without risking the thing being asked about. + + Returns (armed_when_strict_last, armed_when_no_strict_last). + """ + import subprocess + + root = Path(__file__).parent + probe = "tests/recall/test_store_isolation_guard.py::test_the_data_root_guard_is_armed_by_default" + + def _run(*flags): + out = subprocess.run( + [sys.executable, "-m", "pytest", probe, *flags, "-q", "--tb=no", + "-p", "no:cacheprovider"], + cwd=root, capture_output=True, text=True, timeout=300, + ).stdout + # The witness PASSES when armed and SKIPS when deliberately disarmed, + # so the outcome word is the measurement. + return "passed" in out and "skipped" not in out + + return ( + _run("--no-strict-recall-data-root", "--strict-recall-data-root"), + _run("--strict-recall-data-root", "--no-strict-recall-data-root"), + ) + + +@pytest.fixture +def install_policy(): + """The real arming routine, for the witness that the escape hatch is wired.""" + return _install_policy + + @pytest.fixture def isolation_policy(): """The live policy object, for witnesses that inspect the harness itself.""" diff --git a/tests/recall/test_codex.py b/tests/recall/test_codex.py index 452a96dc..9d7d776c 100644 --- a/tests/recall/test_codex.py +++ b/tests/recall/test_codex.py @@ -1,9 +1,10 @@ """Tests for synapt.recall.codex — Codex CLI transcript parsing.""" import json -import os import tempfile import unittest + +from _isolation_helpers import owned_store from pathlib import Path from synapt.recall.codex import ( @@ -575,24 +576,15 @@ class TestTheBuildPreCheckReadsTheCodexArm(unittest.TestCase): def setUp(self): # cmd_build resolves the recall data root implicitly, so without an - # owned root this drives the pre-check against the operator's live - # store (Ref #967). unittest.TestCase cannot take the shared fixture, - # so the override is set here and restored in tearDown. - self._tmp = tempfile.mkdtemp() - self._prev = os.environ.get("SYNAPT_RECALL_ROOT") - self._prev_wt = os.environ.get("SYNAPT_RECALL_WORKTREE") - os.environ["SYNAPT_RECALL_ROOT"] = self._tmp - os.environ["SYNAPT_RECALL_WORKTREE"] = "pytest-owned" + # owned root this drives the pre-check against a live store (Ref #967). + # unittest.TestCase cannot take the shared fixture, so it uses the + # shared helper — this was a hand-rolled two-variable save/restore + # until now, which is exactly the duplication owned_store exists to + # remove. Consolidated: one restore path, not a per-class copy. + self._store = owned_store() def tearDown(self): - for name, prev in ( - ("SYNAPT_RECALL_ROOT", self._prev), - ("SYNAPT_RECALL_WORKTREE", self._prev_wt), - ): - if prev is None: - os.environ.pop(name, None) - else: - os.environ[name] = prev + self._store.restore() def _run_precheck(self, has_codex: bool): """Drive cmd_build's pre-check with everything downstream stubbed.""" diff --git a/tests/recall/test_store_isolation_guard.py b/tests/recall/test_store_isolation_guard.py index 71ac0175..4e2cc71b 100644 --- a/tests/recall/test_store_isolation_guard.py +++ b/tests/recall/test_store_isolation_guard.py @@ -26,6 +26,7 @@ import os import pathlib +import sys from pathlib import Path import pytest @@ -154,6 +155,105 @@ def test_the_guard_is_armed_with_no_per_test_fixture_at_all( channel_mod._guard_store_path("witness", protected_channel_root / "dev.jsonl") +def test_the_data_root_guard_is_armed_by_default( + request, store_isolation_error, monkeypatch +): + """The data-root guard is installed with no flag passed. + + This is the witness the flip rests on, and it is deliberately a DIRECT + observation of the policy rather than a comparison between two run modes. + Before the burn-down, running with the flag differed from running without + it, and that difference was the proof the flag was wired. Now the two agree + — which is both the success criterion and the exact signature of a guard + that has stopped working. Modal agreement cannot tell those apart any more, + so the evidence has to come from here. + """ + if not request.config.getoption("strict_recall_data_root"): + # Discriminate on the ARGV signal, not on the resolved option. Those + # two agree today and come apart in exactly the case that matters: if + # the default is ever silently reverted to off, the resolved option + # reads False with no flag passed, and skipping here would announce + # "deliberately disarmed" about a disarm nobody requested. That is the + # skip LYING rather than merely going quiet — a false reason is worse + # than no reason, because it explains away the very thing it should + # surface. + assert "--no-strict-recall-data-root" in sys.argv, ( + "the data-root guard is disarmed but nobody passed the flag — the " + "default has been reverted; this must fail rather than skip" + ) + pytest.skip("deliberately disarmed via --no-strict-recall-data-root") + + assert core_mod._data_root_policy is not None, ( + "the data-root guard must be armed without passing any flag" + ) + + monkeypatch.delenv("SYNAPT_RECALL_ROOT", raising=False) + outside = Path(__file__).resolve().parents[2] + + with pytest.raises(store_isolation_error): + core_mod.project_data_dir(outside) + + +def test_the_debug_escape_hatch_is_wired_end_to_end(request, install_policy): + """``--no-strict-recall-data-root`` reaches the install decision. + + Two links, asserted separately, because either one alone is satisfiable + without the other: the option must resolve to the same destination the + installer reads, and the installer must actually decline to arm when told + not to. An escape hatch nobody has proven works is worse than none — it + invites the belief that the guard can be stepped around cleanly, and the + first person to need it finds out otherwise under pressure. + + Deliberately not a nested pytest run. ``pytester`` executes in-process and + shares module globals, which is how the guard silently disarmed itself + once already; a second nested run to test disarming would be the same + hazard aimed at the thing it broke before. + """ + # Link 1: the option exists and resolves to the destination the installer + # reads. Under --no-strict-recall-data-root that destination is False, + # which is itself the proof the flag reaches it. + armed = request.config.getoption("strict_recall_data_root") + assert armed is ("--no-strict-recall-data-root" not in sys.argv) + + # Link 2: told not to arm, the installer does not arm. + previous = core_mod.set_data_root_policy(None) + try: + install_policy(False) + assert core_mod._data_root_policy is None + # Link 3: the hatch is SCOPED. It disarms the data-root guard and + # nothing else. Without this, the escape could widen into a full + # disarm — taking the channel guard, which is #955's contract and was + # never opt-in — and every other witness here would still pass, because + # they arm their own policy or assert against a decoy. An escape hatch + # nobody has bounded is a hatch that grows. + assert channel_mod._store_path_policy is not None, ( + "the data-root escape must not disarm the channel guard" + ) + install_policy(True) + assert core_mod._data_root_policy is not None + finally: + core_mod.set_data_root_policy(previous) + + +def test_passing_both_flags_resolves_last_wins(pytester_precedence): + """With both flags given, the LAST one decides — pinned, not inherited. + + Found in review. The behaviour was already last-wins, but by argparse + accident rather than by decision: undocumented, untested, and therefore + free to change under a dependency bump with nothing going red. That is the + same shape as the rest of this file — a behaviour nobody chose, invisible + until it decides something. + + The concrete case is a CI base config carrying one flag and a job appending + the other, where the outcome is settled by argument order nobody wrote + down. Last-wins is the conventional answer and a good one; this makes it a + decision by pinning it, and the docstring on the option now says so. + """ + armed_last, disarmed_last = pytester_precedence + assert armed_last is True, "--strict last must arm" + assert disarmed_last is False, "--no-strict last must disarm" + + def test_layer_one_covers_the_import_window(isolation_policy): """The pre-collection default exists, even though tests do not run under it. From 351ae274d09dc36824f6638c7ab66866ab5c9741 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Sun, 9 Aug 2026 12:02:30 -0500 Subject: [PATCH 05/19] fix(recall): a member's store belongs to the workspace it is a member of Store resolution consulted the gr2 workspace marker before the gr1 membership marker, so a directory carrying both resolved to itself and its declared membership was never read. Its recall data landed where no other surface in that workspace looks, and nothing anywhere reported a problem. The two markers answer different questions. ".grip" asks "am I a workspace containing units?"; ".gitgrip/griptree.json" asks "whose larger whole am I a part of?". Store resolution needs the second, because a member's data belongs to the workspace it is a member of. Membership is now consulted first, and where it is asserted but cannot be verified, resolution falls through to the locality marker rather than reporting nothing. Reordering alone would have made that population WORSE: such a directory used to short-circuit on ".grip" and resolve to itself, and would instead have resolved to nothing -- fragmenting a store that previously cohered, which is the same failure this change exists to prevent. Where membership cannot be verified and there is no locality evidence either, the result is None exactly as before, and the walk deliberately does not continue upward. Continuing would find a better answer for that case, and that is a separate change with its own evidence. The property this one claims is that every directory either improves or is unchanged and nothing else moves -- which stops being checkable the moment it also improves an unrelated case. Five tests, none of which the suite previously had. Every existing case builds one marker or the other, so all 38 passed identically with and without the reordering: the suite could not distinguish fixed from unfixed. The new cases fail with AssertionError against the behaviour they replace, naming the wrong root. Two of the five are controls rather than claims -- one pins that locality is narrowed and not removed, the other pins the no-locality case as unchanged and deliberately NOT improved. --- src/synapt/recall/core.py | 54 +++++++++++--- tests/recall/test_gripspace.py | 127 +++++++++++++++++++++++++++++++++ 2 files changed, 172 insertions(+), 9 deletions(-) diff --git a/src/synapt/recall/core.py b/src/synapt/recall/core.py index 3a7c4e04..556bf4d9 100644 --- a/src/synapt/recall/core.py +++ b/src/synapt/recall/core.py @@ -4359,13 +4359,6 @@ def _find_gripspace_root(path: Path) -> Path | None: home = Path.home().resolve() while current != current.parent: - # gr2 owns one .grip/ namespace at the workspace root. Spawned units - # live beneath it, so recognizing that marker makes their shared index - # and knowledge land at the containing workspace instead of at each - # unit cwd. This composes with the existing gr1 detector below. - if (current / ".grip").is_dir(): - _gripspace_cache[cache_key] = (current, time.monotonic()) - return current gitgrip = current / ".gitgrip" if gitgrip.is_dir(): # Linked griptree has griptree.json (singular) — always resolve @@ -4373,12 +4366,55 @@ def _find_gripspace_root(path: Path) -> Path | None: # happens when `gr` clones the full directory structure). if (gitgrip / "griptree.json").exists(): root = _resolve_griptree_parent(current) - _gripspace_cache[cache_key] = (root, time.monotonic()) - return root + if root is not None: + _gripspace_cache[cache_key] = (root, time.monotonic()) + return root + # Membership is ASSERTED here but could not be VERIFIED. Use + # the strongest signal that actually was verified: ".grip" is + # positive evidence this directory is a workspace, so fall + # through to it rather than reporting nothing. + # + # Without this, reordering the two markers would make things + # WORSE for exactly this population: such a directory used to + # short-circuit on ".grip" and resolve to itself, and would + # instead resolve to nothing — fragmenting a store that + # previously cohered, which is the failure this function is + # being changed to prevent. + if (current / ".grip").is_dir(): + _gripspace_cache[cache_key] = (current, time.monotonic()) + return current + # No membership we can verify and no locality evidence either. + # Return None EXACTLY as before, and deliberately do NOT keep + # walking upward. Continuing the walk might well find a better + # answer, but it would be a NEW behaviour for a case this + # change is not about, and it would break the property that + # makes this change checkable: every directory either improves + # or is unchanged, and nothing else moves. + _gripspace_cache[cache_key] = (None, time.monotonic()) + return None # Gripspace root has only griptrees.json (plural), no singular if (gitgrip / "griptrees.json").exists(): _gripspace_cache[cache_key] = (current, time.monotonic()) return current + + # MEMBERSHIP BEATS LOCALITY. gr2 owns one .grip/ namespace at the + # workspace root, and spawned units live beneath it, so recognizing + # that marker makes their shared index and knowledge land at the + # containing workspace instead of at each unit cwd. That is still + # true, and it is why this check exists. + # + # It is consulted AFTER the gr1 membership markers above, because the + # two answer different questions: ".grip" asks "am I a workspace + # containing units?", while .gitgrip/griptree.json asks "whose larger + # whole am I a part of?". A directory can hold both, and store + # resolution needs the second — a member's data belongs to the + # workspace it is a member of. Checked first, ".grip" would shadow the + # membership claim entirely: the directory resolves to itself, its + # data lands where no other surface in that workspace looks, and + # nothing anywhere reports a problem. + if (current / ".grip").is_dir(): + _gripspace_cache[cache_key] = (current, time.monotonic()) + return current # Don't walk above $HOME if current == home: break diff --git a/tests/recall/test_gripspace.py b/tests/recall/test_gripspace.py index 8a47d6f0..1ba223c3 100644 --- a/tests/recall/test_gripspace.py +++ b/tests/recall/test_gripspace.py @@ -150,6 +150,133 @@ def test_linked_griptree_data_dir_matches_parent(self, tmp_path): result = project_data_dir(griptree) assert result == grip / ".synapt" / "recall" + def _make_member_carrying_both_markers(self, tmp_path: Path) -> tuple[Path, Path]: + """A directory that is BOTH a gr2 workspace and a declared member. + + This combination is not hypothetical: a workspace acquires the gr2 + marker when it is managed by gr2, and keeps its membership marker + because it is still a griptree of a larger gripspace. Every existing + test in this class builds one marker or the other, which is why the + suite could not see the ordering between them. + """ + grip = _make_gripspace(tmp_path) + main_repo = grip / "my-repo" + main_repo.mkdir() + git_dir = main_repo / ".git" + git_dir.mkdir() + worktrees_dir = git_dir / "worktrees" / "dev" + worktrees_dir.mkdir(parents=True) + + member = tmp_path / "dev-tree" + member.mkdir() + (member / ".gitgrip").mkdir() + (member / ".gitgrip" / "griptree.json").write_text( + '{"branch": "dev", "path": "' + str(member) + '"}' + ) + linked_repo = member / "my-repo" + linked_repo.mkdir() + (linked_repo / ".git").write_text(f"gitdir: {worktrees_dir}\n") + # ...and it is also a gr2-managed workspace. + (member / ".grip").mkdir() + return grip, member + + def test_membership_beats_locality(self, tmp_path): + """Both markers present: resolve to the whole, not to the part. + + The two markers answer different questions. ".grip" asks "am I a + workspace containing units?"; the griptree marker asks "whose larger + whole am I part of?". Store resolution needs the second, because a + member's data belongs to the workspace it is a member of — resolving + to itself strands that data where no other surface in the workspace + looks, and reports nothing. + """ + grip, member = self._make_member_carrying_both_markers(tmp_path) + + assert _find_gripspace_root(member) == grip + + def test_membership_beats_locality_at_the_data_dir(self, tmp_path): + """The same claim at the level the caller actually consumes. + + Asserting the resolver alone would leave the reader-visible effect + unproven, and the reader-visible effect is the whole point: this is + the path a store lands on. + """ + grip, member = self._make_member_carrying_both_markers(tmp_path) + + assert project_data_dir(member) == grip / ".synapt" / "recall" + + def test_gr2_locality_still_applies_without_a_membership_claim(self, tmp_path): + """The control for the two above: locality is narrowed, not removed. + + A gr2 workspace with no membership marker must still resolve to + itself. Without this, the two tests above would also pass if the + ".grip" branch had simply been deleted. + """ + workspace = _make_gr2_workspace(tmp_path) + unit_home = workspace / "units" / "u_one" / "home" + unit_home.mkdir(parents=True) + + assert _find_gripspace_root(workspace) == workspace + assert _find_gripspace_root(unit_home) == workspace + + def _make_unresolvable_member( + self, parent: Path, with_grip: bool, name: str = "member" + ) -> Path: + """A directory declaring membership whose parent cannot be resolved. + + It has sub-directories but none carries a `.git` *file*, so the + worktree-pointer walk that resolves a griptree back to its parent + finds nothing to follow and reports no parent. + """ + member = parent / name + member.mkdir() + (member / ".gitgrip").mkdir() + (member / ".gitgrip" / "griptree.json").write_text('{"branch": "dev"}') + (member / "docs").mkdir() + if with_grip: + (member / ".grip").mkdir() + return member + + def test_unresolvable_membership_falls_back_to_locality(self, tmp_path): + """THE REGRESSION WITNESS — consulting membership first must not strand. + + Membership is asserted here and cannot be verified. Before the marker + order changed, such a directory short-circuited on `.grip` and + resolved to itself; consulting membership first would instead resolve + it to nothing, and its sub-directories would each land on a store of + their own. That fragments a store that previously cohered, which is + the exact failure this function is being changed to prevent. + + So the fall-through is not a nicety: without it this change is a + regression for this population. Pinned so no later edit can quietly + re-break it. + """ + member = self._make_unresolvable_member(tmp_path, with_grip=True) + deep = member / "docs" / "deep" + deep.mkdir(parents=True) + + assert _find_gripspace_root(member) == member + assert _find_gripspace_root(deep) == member + assert project_data_dir(deep) == member / ".synapt" / "recall" + + def test_unresolvable_membership_without_locality_does_not_walk_upward( + self, tmp_path + ): + """The third case, pinned deliberately UNCHANGED rather than improved. + + Membership asserted, unverifiable, and no locality evidence either. + There IS a gripspace root above it, so continuing the walk would find + a better answer than `None` — and that is precisely why this asserts + `None`. A real improvement here belongs to the change that can bring + its own evidence for it. Cause 1's claim is that every directory + either improves or is unchanged and nothing else moves; the moment it + also improves an unrelated case, that claim stops being checkable. + """ + grip = _make_gripspace(tmp_path) + member = self._make_unresolvable_member(grip, with_grip=False) + + assert _find_gripspace_root(member) is None + def test_linked_griptree_no_subrepos_returns_none(self, tmp_path): """A linked griptree with no sub-repos can't resolve — returns None.""" griptree = tmp_path / "orphan-tree" From af74c53e3cc7f1a9186795b67d82f7de0a9ef884 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 13 Aug 2026 06:01:56 -0500 Subject: [PATCH 06/19] test: TDD spec for build idempotence A second consecutive `build` over an unchanged store must do ~zero work, run fast, and say that it skipped. It does not today. Measured on a frozen 4-transcript corpus (39MB, 1,709 chunks) with mtimes pinned so "unchanged" was a controlled fact rather than an assumption, a run that parsed ZERO files still took 40.9s. The build has exactly one change-detector, it works, and it guards the cheapest stage; every expensive stage downstream is unconditional. Stage attribution on that do-nothing run: parse 1.2s, full FTS5 re-save 1.2s, full re-cluster 0.6s, re-tag every cluster, timeline rebuild, and then 37.7s (92%) in the LLM cluster-summary backlog grinder, which loads a model and makes network round-trips on every build regardless of whether anything changed. Specifies, as failing tests: - parse-skip keys scoped by source dir rather than basename alone. The manifest is one flat list spanning every archive dir while the skip dict is keyed on basename, so same-named files shadow each other and the loser can never match. - archive freshness on size OR mtime. Freshness is size-only today, so a same-size content edit is never re-archived and therefore never re-indexed. - an input signature spanning transcripts, channels and journals, with the no-op decision derived from it and surviving a manifest round trip. - the summary grinder off the build path and onto an explicit, bounded `maintain` command that reports its remaining backlog rather than draining it silently. - a no-op run that says it is a no-op. - incremental as the default with `--full` as the explicit opt-in, and the CLI and MCP defaults agreeing (they currently disagree). Every skip assertion is paired with a negative control that changes exactly one input and requires the build to notice, so the suite cannot be satisfied by an implementation that simply skips everything. The controls cover a changed transcript, a new channel message, a new journal entry, a newly arrived file, and the skip message itself never appearing on a run that had work to do. 20 failing, 6 passing; the 6 are regression guards and controls that already hold on current behavior. Failing tests are expected on a feature branch. Unrelated and pre-existing, recorded so the next reader does not attribute it here: four tests in test_attribution.py fail on clean dev as well, because TranscriptChunk reads SYNAPT_AGENT_ID from the environment and the test does not isolate it. Verified by running them against origin/dev with this file absent. Not fixed here; out of scope. Premium boundary: OSS. Index build and retrieval only, no identity or org semantics. --- tests/recall/test_build_idempotence.py | 582 +++++++++++++++++++++++++ 1 file changed, 582 insertions(+) create mode 100644 tests/recall/test_build_idempotence.py diff --git a/tests/recall/test_build_idempotence.py b/tests/recall/test_build_idempotence.py new file mode 100644 index 00000000..7aef5b25 --- /dev/null +++ b/tests/recall/test_build_idempotence.py @@ -0,0 +1,582 @@ +"""TDD spec: `synapt recall build` must be idempotent. + +Contract under test +------------------- +A second consecutive build over an unchanged store must do ~zero work, run +fast, and SAY that it skipped. Today it does not: measured on a frozen +4-transcript corpus (39MB, 1,709 chunks) with mtimes pinned, a run that parsed +ZERO files still took 40.9s. The build has exactly one change-detector +(``core.build_index``, mtime+size) and it guards the cheapest stage; every +expensive stage downstream is unconditional. + +Why the controls in here are not optional +----------------------------------------- +The cheapest way to make every "did it skip?" assertion pass is to skip +unconditionally, which would turn a slow build into a broken one. So each skip +assertion is paired with a NEGATIVE control that changes exactly one input and +demands the build notice. A suite that only proves skipping is a suite that +cannot fail the worst available implementation. + +That pairing is not hypothetical caution — the harness that produced the +measurements above shipped a bad control first: it touched an mtime against a +freshness check keyed on SIZE, so it was aimed at a quantity the code does not +read, and it "passed" while proving nothing. +""" + +from __future__ import annotations + +import json +import os +import time +from pathlib import Path + +import pytest + +from conftest import assistant_entry, user_text_entry, write_jsonl + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _transcript(path: Path, *, turns: int = 2, prefix: str = "q") -> Path: + """Write a small but genuinely parseable Claude Code transcript.""" + entries = [] + for i in range(turns): + entries.append( + user_text_entry( + f"{prefix} question {i}", + uuid=f"{prefix}-u{i}", + ts=f"2026-03-01T10:{i:02d}:00Z", + ) + ) + entries.append( + assistant_entry( + text=f"{prefix} answer {i}", + uuid=f"{prefix}-a{i}", + ts=f"2026-03-01T10:{i:02d}:30Z", + ) + ) + write_jsonl(path, entries) + return path + + +def _set_mtime(path: Path, when: float) -> None: + os.utime(path, (when, when)) + + +def _manifest_entry(path: Path, *, source_dir: Path | None = None) -> dict: + st = path.stat() + entry = {"name": path.name, "mtime": st.st_mtime, "size": st.st_size} + if source_dir is not None: + entry["dir"] = source_dir.name + return entry + + +# =========================================================================== +# A. Parse-skip keying: the detector must be scoped by SOURCE DIR +# =========================================================================== +# +# cli.py writes ONE flat `source_files` list spanning every worktree archive +# dir, while core.build_index keys `already_indexed` on the BASENAME alone, so +# same-named files from different worktrees overwrite each other and the loser +# can never match. Verified on the live manifest: 18 entries, 14 distinct +# basenames, 4 Codex rollout files shadowed (identical size, mtime differing by +# ~107s) and re-parsing on every incremental build, permanently. + +def test_same_basename_in_two_source_dirs_both_skip(tmp_path): + """Two worktrees archiving the same session name must BOTH skip. + + This is the live defect in miniature: identical size, differing mtime. + """ + from synapt.recall.core import build_index + + dir_a = tmp_path / "worktree-a" + dir_b = tmp_path / "worktree-b" + dir_a.mkdir() + dir_b.mkdir() + + name = "rollout-2026-03-20T15-11-29-019d0ca8.jsonl" + file_a = _transcript(dir_a / name, prefix="a") + file_b = _transcript(dir_b / name, prefix="a") # same bytes, same size + assert file_a.stat().st_size == file_b.stat().st_size, "fixture must collide on size" + + # Distinct mtimes, exactly like the live manifest's ~107s spread. + _set_mtime(file_a, 1785988016.0) + _set_mtime(file_b, 1785988123.0) + + manifest = { + "source_files": [ + _manifest_entry(file_a, source_dir=dir_a), + _manifest_entry(file_b, source_dir=dir_b), + ] + } + + idx_a = build_index(dir_a, incremental_manifest=manifest) + idx_b = build_index(dir_b, incremental_manifest=manifest) + + assert idx_a.chunks == [], f"dir_a re-parsed despite an exact manifest match: {len(idx_a.chunks)} chunks" + assert idx_b.chunks == [], f"dir_b re-parsed despite an exact manifest match: {len(idx_b.chunks)} chunks" + + +def test_changed_file_still_reparses_under_dir_scoped_keys(tmp_path): + """NEGATIVE CONTROL for the test above. + + Dir-scoping must not be implemented by making everything match. Change one + file's content and it must come back. + """ + from synapt.recall.core import build_index + + dir_a = tmp_path / "worktree-a" + dir_b = tmp_path / "worktree-b" + dir_a.mkdir() + dir_b.mkdir() + + name = "shared-session.jsonl" + file_a = _transcript(dir_a / name, prefix="a") + file_b = _transcript(dir_b / name, prefix="a") + _set_mtime(file_a, 1785988016.0) + _set_mtime(file_b, 1785988123.0) + + manifest = { + "source_files": [ + _manifest_entry(file_a, source_dir=dir_a), + _manifest_entry(file_b, source_dir=dir_b), + ] + } + + # Grow dir_a's copy only. + _transcript(file_a, turns=5, prefix="a") + + idx_a = build_index(dir_a, incremental_manifest=manifest) + idx_b = build_index(dir_b, incremental_manifest=manifest) + + assert idx_a.chunks, "dir_a changed and must be re-parsed" + assert idx_b.chunks == [], "dir_b did not change and must still skip" + + +def test_legacy_manifest_without_dir_field_still_skips(tmp_path): + """Backward compatibility: manifests written before dir-scoping. + + An existing store's manifest has no `dir` key. Upgrading must not force a + one-time full rebuild of every project in the wild. + """ + from synapt.recall.core import build_index + + src = tmp_path / "archive" + src.mkdir() + f = _transcript(src / "legacy-session.jsonl") + _set_mtime(f, 1785988016.0) + + legacy = {"source_files": [{"name": f.name, "mtime": f.stat().st_mtime, "size": f.stat().st_size}]} + + idx = build_index(src, incremental_manifest=legacy) + assert idx.chunks == [], "a legacy (dir-less) manifest entry must still be honoured" + + +# =========================================================================== +# B. Archive freshness: size OR mtime +# =========================================================================== +# +# archive.py:704 skips when `src_size == dst_size`, so a same-size content edit +# is never re-archived and therefore never re-indexed. + +def test_archive_refreshes_on_same_size_newer_mtime(tmp_path): + from synapt.recall.archive import archive_transcripts + from synapt.recall.core import project_archive_dir + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + + f = source / "session.jsonl" + f.write_text('{"a": "0000000000"}\n') + archive_transcripts(project, source) + + archived = project_archive_dir(project) / "session.jsonl" + assert archived.exists(), "fixture failed: first archive did not land" + original_size = archived.stat().st_size + + # Same byte count, different content, newer mtime. + f.write_text('{"a": "1111111111"}\n') + assert f.stat().st_size == original_size, "fixture must hold size constant" + _set_mtime(f, time.time() + 10) + + archive_transcripts(project, source) + assert archived.read_text() == f.read_text(), ( + "same-size content edit was never re-archived, so it can never be re-indexed" + ) + + +def test_archive_skips_when_size_and_mtime_both_match(tmp_path): + """NEGATIVE CONTROL: freshness must not degrade into copy-always.""" + from synapt.recall.archive import archive_transcripts + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "session.jsonl") + + archive_transcripts(project, source) + second = archive_transcripts(project, source) + assert second == [], f"unchanged source was re-archived: {second}" + + +def test_archive_still_preserves_larger_archive_when_source_shrinks(tmp_path): + """Regression guard on existing behavior. + + A `/clear` truncates the live transcript. The archive holds the longer + history and must not be overwritten by the shorter one. + """ + from synapt.recall.archive import archive_transcripts + from synapt.recall.core import project_archive_dir + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + + f = _transcript(source / "session.jsonl", turns=6) + archive_transcripts(project, source) + archived = project_archive_dir(project) / "session.jsonl" + long_size = archived.stat().st_size + + _transcript(f, turns=1) # truncated by /clear + _set_mtime(f, time.time() + 10) + archive_transcripts(project, source) + + assert archived.stat().st_size == long_size, "shorter source overwrote a longer archive" + + +# =========================================================================== +# C. The nothing-changed signal +# =========================================================================== +# +# `incremental` currently gates exactly one call site. A real no-op needs a +# signal covering EVERY build input: transcripts, channels and journals. + +def test_identical_inputs_are_a_noop(tmp_path): + from synapt.recall.build_delta import compute_input_signature, is_noop + + src = tmp_path / "archive" + src.mkdir() + _transcript(src / "s1.jsonl") + channels = tmp_path / "channels" + channels.mkdir() + (channels / "dev.jsonl").write_text('{"id":"m1","body":"hello"}\n') + journal = tmp_path / "journal.jsonl" + journal.write_text('{"session_id":"s1","focus":"x"}\n') + + first = compute_input_signature([src], channels, [journal]) + second = compute_input_signature([src], channels, [journal]) + + assert is_noop(first, second) is True + + +def test_no_previous_signature_is_never_a_noop(tmp_path): + """A first build, or a build after a corrupt manifest, must do the work.""" + from synapt.recall.build_delta import compute_input_signature, is_noop + + src = tmp_path / "archive" + src.mkdir() + _transcript(src / "s1.jsonl") + + assert is_noop(None, compute_input_signature([src], None, [])) is False + + +@pytest.mark.parametrize("mutate", ["transcript", "channel", "journal"]) +def test_any_changed_input_defeats_the_noop(tmp_path, mutate): + """NEGATIVE CONTROLS, one per input class. + + These are the tests that stop "make it fast" from becoming "make it lie". + A signal that only watches transcripts would let a new channel message or a + fresh journal entry go unindexed while the build cheerfully reports it is + up to date — which is worse than being slow, because it is silent. + """ + from synapt.recall.build_delta import compute_input_signature, is_noop + + src = tmp_path / "archive" + src.mkdir() + t = _transcript(src / "s1.jsonl") + channels = tmp_path / "channels" + channels.mkdir() + ch = channels / "dev.jsonl" + ch.write_text('{"id":"m1","body":"hello"}\n') + journal = tmp_path / "journal.jsonl" + journal.write_text('{"session_id":"s1","focus":"x"}\n') + + before = compute_input_signature([src], channels, [journal]) + + if mutate == "transcript": + _transcript(t, turns=5) + elif mutate == "channel": + with ch.open("a") as fh: + fh.write('{"id":"m2","body":"a new message"}\n') + else: + with journal.open("a") as fh: + fh.write('{"session_id":"s2","focus":"y"}\n') + + after = compute_input_signature([src], channels, [journal]) + assert is_noop(before, after) is False, f"a changed {mutate} must defeat the no-op" + + +def test_new_file_appearing_defeats_the_noop(tmp_path): + """A signature that only hashes known files misses arrivals.""" + from synapt.recall.build_delta import compute_input_signature, is_noop + + src = tmp_path / "archive" + src.mkdir() + _transcript(src / "s1.jsonl") + before = compute_input_signature([src], None, []) + + _transcript(src / "s2.jsonl", prefix="b") + after = compute_input_signature([src], None, []) + + assert is_noop(before, after) is False, "a newly arrived transcript must defeat the no-op" + + +def test_signature_survives_a_manifest_round_trip(tmp_path): + """The signal is only useful if it persists between processes.""" + from synapt.recall.build_delta import ( + compute_input_signature, + is_noop, + signature_from_manifest, + signature_to_manifest, + ) + + src = tmp_path / "archive" + src.mkdir() + _transcript(src / "s1.jsonl") + sig = compute_input_signature([src], None, []) + + # Through JSON, because that is how it reaches SQLite metadata. + restored = signature_from_manifest(json.loads(json.dumps(signature_to_manifest(sig)))) + + assert restored is not None + assert is_noop(restored, compute_input_signature([src], None, [])) is True + + +def test_signature_absent_from_manifest_returns_none(tmp_path): + from synapt.recall.build_delta import signature_from_manifest + + assert signature_from_manifest({"source_files": []}) is None + + +# =========================================================================== +# D. The build must never grind LLM summaries +# =========================================================================== +# +# `upgrade_large_cluster_summaries` is an unconditional backlog grinder: +# max_upgrades=5 per build, loading flan-t5-base and making ~11 huggingface.co +# round-trips every run. Measured at 37.7s of a 40.9s do-nothing build (92%). +# On a real 11.6k-chunk store measured during this work, 970 clusters were +# pending — 194 further builds just to drain the backlog, while newly arriving +# chunks refill it. It belongs on an explicit maintenance command, not on the +# build path. + +def test_build_never_calls_the_summary_grinder(tmp_path, monkeypatch): + import synapt.recall.clustering as clustering + from synapt.recall.cli import _archive_and_build + + calls: list[dict] = [] + + def _spy(db, min_chunks=5, max_upgrades=5): + calls.append({"min_chunks": min_chunks, "max_upgrades": max_upgrades}) + return 0 + + monkeypatch.setattr(clustering, "upgrade_large_cluster_summaries", _spy) + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + + assert calls == [], f"build invoked the summary grinder: {calls}" + + +# =========================================================================== +# E. `synapt maintain` — the grinder's new, explicit home +# =========================================================================== + +def test_maintain_subcommand_is_registered(): + """`make_parser()` does not exist yet — the parser is built inline inside + `main()`, which is why no CLI default is currently testable at all. + Extracting it is part of this change, not incidental refactoring.""" + from synapt.recall.cli import make_parser + + actions = [a for a in make_parser()._actions if hasattr(a, "choices") and a.choices] + commands = set() + for a in actions: + commands.update(a.choices or {}) + assert "maintain" in commands, f"no `maintain` subcommand; got {sorted(commands)}" + + +def test_maintain_parser_accepts_limit(): + from synapt.recall.cli import make_parser + + args = make_parser().parse_args(["maintain", "--limit", "7"]) + assert args.command == "maintain" + assert args.limit == 7 + + +def test_maintain_has_a_default_limit(): + from synapt.recall.cli import make_parser + + args = make_parser().parse_args(["maintain"]) + assert isinstance(args.limit, int) and args.limit > 0, ( + "maintain must be bounded by default; an unbounded grind is what we just removed" + ) + + +def test_maintain_passes_the_limit_through_and_reports_backlog(tmp_path, monkeypatch, capsys): + """The backlog must stay visible. Draining it silently would be the + regression this change is meant to avoid.""" + import synapt.recall.clustering as clustering + from synapt.recall.cli import cmd_maintain, make_parser + + seen: list[int] = [] + + def _spy(db, min_chunks=5, max_upgrades=5): + seen.append(max_upgrades) + return 3 + + monkeypatch.setattr(clustering, "upgrade_large_cluster_summaries", _spy) + monkeypatch.chdir(tmp_path) + + project = tmp_path + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + from synapt.recall.cli import _archive_and_build + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=False) + + args = make_parser().parse_args(["maintain", "--limit", "3"]) + cmd_maintain(args) + + out = capsys.readouterr().out.lower() + assert seen == [3], f"maintain did not pass --limit through: {seen}" + assert "remaining" in out, "maintain must report the remaining backlog, not drain it silently" + + +# =========================================================================== +# F. A no-op run must SAY it is a no-op +# =========================================================================== +# +# A fast build and a broken build look identical from the outside unless the +# build states which stages it skipped and why. + +def test_second_build_reports_that_it_skipped(tmp_path, capsys): + from synapt.recall.cli import _archive_and_build + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + capsys.readouterr() + + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + out = capsys.readouterr().out.lower() + + assert "up to date" in out, f"a no-op build said nothing about being a no-op:\n{out}" + assert "skip" in out, f"a no-op build did not report skipped stages:\n{out}" + + +def test_second_build_after_a_change_does_not_claim_to_be_up_to_date(tmp_path, capsys): + """NEGATIVE CONTROL for the message itself. + + The skip line is load-bearing: an operator reads it and stops looking. It + must never appear on a run that had work to do. + """ + from synapt.recall.cli import _archive_and_build + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + t = _transcript(source / "s1.jsonl", turns=8) + + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + capsys.readouterr() + + _transcript(t, turns=20) + _set_mtime(t, time.time() + 10) + + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + out = capsys.readouterr().out.lower() + + assert "up to date" not in out, f"build claimed 'up to date' after real work:\n{out}" + + +def test_noop_build_still_returns_a_usable_index(tmp_path): + """Skipping stages must not degrade the return contract. + + Callers (MCP recall_build, the hooks, setup) read `.stats()` off the + result. A no-op that returns None would turn a fast path into a crash. + """ + from synapt.recall.cli import _archive_and_build + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + first = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + second = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + + assert second is not None, "no-op build returned None; callers dereference this" + assert second.stats()["chunk_count"] == first.stats()["chunk_count"] + + +# =========================================================================== +# G. Default flip — incremental by default, --full the explicit opt-in +# =========================================================================== +# +# Lands as its own commit. The CLI defaults `--incremental` to False while MCP +# recall_build already defaults it True: same operation, opposite defaults +# depending on the surface you reach through. + +def test_build_defaults_to_incremental(): + from synapt.recall.cli import make_parser + + args = make_parser().parse_args(["build"]) + assert getattr(args, "full", False) is False + assert args.incremental is True, "bare `synapt build` must not rewrite everything" + + +def test_full_flag_forces_a_full_rebuild(): + from synapt.recall.cli import make_parser + + args = make_parser().parse_args(["build", "--full"]) + assert args.incremental is False, "--full must be the explicit opt-out from incremental" + + +def test_incremental_flag_still_accepted_for_compatibility(): + """Scripts and hooks in the wild pass --incremental explicitly.""" + from synapt.recall.cli import make_parser + + args = make_parser().parse_args(["build", "--incremental"]) + assert args.incremental is True + + +def test_cli_and_mcp_defaults_agree(): + """The divergence that made this defect hard to see from either side.""" + import inspect + + from synapt.recall.cli import make_parser + from synapt.recall.server import recall_build + + cli_default = make_parser().parse_args(["build"]).incremental + mcp_default = inspect.signature(recall_build).parameters["incremental"].default + assert cli_default == mcp_default, ( + f"CLI default ({cli_default}) and MCP default ({mcp_default}) disagree" + ) From 98c81f525dc85b6bb61e908f4e0f1c20bbce3427 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 13 Aug 2026 07:47:41 -0500 Subject: [PATCH 07/19] docs: provenance README for tests/fixtures/identify Declares every fixture in the directory synthetic and names the rules for adding more, so file names that describe test roles are never again read as captured data. Co-Authored-By: Claude Fable 5 --- tests/fixtures/identify/README.md | 33 +++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 tests/fixtures/identify/README.md diff --git a/tests/fixtures/identify/README.md b/tests/fixtures/identify/README.md new file mode 100644 index 00000000..7b753168 --- /dev/null +++ b/tests/fixtures/identify/README.md @@ -0,0 +1,33 @@ +# Fixture provenance + +Every file in this directory is **synthetic**, authored for the identify +test cycle. None of it is captured from a real session, journal, store, or +agent. + +The file names describe the **test role** of each fixture, not a data +source: + +- `atlas-journal-structured.json` — exercises the structured-journal *shape* + (named-field entries). It is not any agent's journal. +- `dogfood-journal-slice.jsonl` — exercises the line-delimited journal + *format*. It is not a slice of dogfood data. +- `gold-units.jsonl` / `gold-source-map.json` — hand-authored expected + outputs for the gold tests. + +Tells that these are synthetic, visible in the bytes: placeholder +timestamps (`2026-01-01T00:00:00+00:00`), empty `session_id` and `branch` +fields, and generic invented engineering scenarios. + +Rules for adding fixtures here: + +1. Synthetic only. Never copy content from a real session, journal, or + store, even "scrubbed" — author fresh content instead. +2. Zero or placeholder coordinates (timestamps, session ids, branches). +3. If a fixture must mirror the *shape* of a real defect, reproduce the + shape with invented content and say so in the fixture's commit message. + +Why this file exists: a public-branch audit flagged this directory because +the file names read as captured data. The audit's resolution confirmed +synthetic provenance from the authoring PR's own record; this README makes +that provenance legible at the directory itself so the names never trigger +an escalation again. From f627586907cf5bd07d72f9b6c5ab0b9d3b14208c Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 6 Aug 2026 18:05:13 -0500 Subject: [PATCH 08/19] =?UTF-8?q?fix(recall):=20code=5Fgit=20hardening=20?= =?UTF-8?q?=E2=80=94=20timeout=20ceiling,=20path-free=20errors,=20pinned?= =?UTF-8?q?=20locale?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit r2's three non-blocking notes from the live-history merge, each with a born-red witness (the four new tests all red against the pre-hardening _run_git before this change): - subprocess.run now carries a 30s ceiling. A live-per-call primitive whose git blocks on an index.lock otherwise hangs the caller with no way out. Timeout surfaces as the same ValueError contract. - the repository path stays out of error messages. An error that can reach a user-facing surface must not disclose local layout. - git runs under LC_ALL=C. Parsers and error assertions match git's English text, and an unpinned locale is green on this machine and red on a non-English one. tests/recall/test_code_git.py: 18 passed (14 existing untouched). Co-Authored-By: Claude Fable 5 --- src/synapt/recall/code_git.py | 33 ++++++++++++----- tests/recall/test_code_git.py | 68 +++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 8 deletions(-) diff --git a/src/synapt/recall/code_git.py b/src/synapt/recall/code_git.py index 6da29dd1..eff78739 100644 --- a/src/synapt/recall/code_git.py +++ b/src/synapt/recall/code_git.py @@ -27,6 +27,7 @@ from dataclasses import dataclass from pathlib import Path +import os import subprocess __all__ = ["BlameSpan", "FileCommit", "blame_range", "file_history"] @@ -62,16 +63,32 @@ class BlameSpan: line_end: int +# A live-per-call primitive needs a ceiling: a git blocked on an index.lock or +# a pathological repository would otherwise hang the caller indefinitely. +_GIT_TIMEOUT_SECONDS = 30.0 + + def _run_git(repo: str | Path, args: list[str]) -> str: - proc = subprocess.run( - ["git", "-C", str(repo), *args], - capture_output=True, - text=True, - ) - if proc.returncode != 0: - raise ValueError( - f"git {args[0]} failed in {repo}: {proc.stderr.strip()}" + # LC_ALL=C pins git's message text: the parsers and error assertions match + # English strings, and an unpinned locale is green on this machine and red + # on a non-English one. + env = {**os.environ, "LC_ALL": "C", "LANG": "C"} + try: + proc = subprocess.run( + ["git", "-C", str(repo), *args], + capture_output=True, + text=True, + timeout=_GIT_TIMEOUT_SECONDS, + env=env, ) + except subprocess.TimeoutExpired: + raise ValueError( + f"git {args[0]} timed out after {_GIT_TIMEOUT_SECONDS:g}s" + ) from None + if proc.returncode != 0: + # The repository path stays out of the message: an error that reaches a + # user-facing surface must not disclose local layout. + raise ValueError(f"git {args[0]} failed: {proc.stderr.strip()}") return proc.stdout diff --git a/tests/recall/test_code_git.py b/tests/recall/test_code_git.py index 83629d7b..4d7fa630 100644 --- a/tests/recall/test_code_git.py +++ b/tests/recall/test_code_git.py @@ -283,3 +283,71 @@ def test_module_imports_no_storage_and_no_memory_layer() -> None: assert "sqlite3" not in imported assert not any(name == "synapt" or name.startswith("synapt.") for name in imported) + + +# --------------------------------------------------------------------------- +# Hardening witnesses — r2's three non-blocking notes on the A5 merge, +# each born red against the pre-hardening `_run_git` (physics 082: a test +# written to expose a known defect is trusted only after it reds against it). +# --------------------------------------------------------------------------- + + +def test_run_git_carries_a_timeout(monkeypatch): + """A live-per-call primitive needs a ceiling: a git blocked on index.lock + or a pathological repo must not hang the caller indefinitely.""" + import synapt.recall.code_git as cg + + seen: dict = {} + + def capture(cmd, **kwargs): + seen.update(kwargs) + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + monkeypatch.setattr(cg.subprocess, "run", capture) + cg._run_git(".", ["log"]) + assert seen.get("timeout") and seen["timeout"] > 0, ( + "no ceiling: a hung git hangs the caller forever" + ) + + +def test_a_timed_out_git_reports_cleanly_without_the_repo_path(tmp_path, monkeypatch): + """Timeout surfaces as the same ValueError contract, path withheld.""" + import synapt.recall.code_git as cg + + def explode(cmd, **kwargs): + raise subprocess.TimeoutExpired(cmd, kwargs.get("timeout") or 1) + + monkeypatch.setattr(cg.subprocess, "run", explode) + with pytest.raises(ValueError) as e: + cg._run_git(tmp_path, ["log"]) + assert "timed out" in str(e.value) + assert str(tmp_path) not in str(e.value) + + +def test_error_message_withholds_the_repo_path(tmp_path): + """The path-disclosure class: an error that can reach a user-facing + surface states what failed, never the local layout it failed in.""" + with pytest.raises(ValueError) as e: + file_history(tmp_path, "anything.py") + assert str(tmp_path) not in str(e.value), ( + "the repo path is embedded in the error message" + ) + + +def test_git_runs_under_a_pinned_locale(monkeypatch): + """Parsers and the error tests above match git's ENGLISH text; an + unpinned locale is green on this machine and red on a non-English one.""" + import synapt.recall.code_git as cg + + seen: dict = {} + + def capture(cmd, **kwargs): + seen.update(kwargs) + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + monkeypatch.setattr(cg.subprocess, "run", capture) + cg._run_git(".", ["log"]) + env = seen.get("env") + assert env is not None and env.get("LC_ALL") == "C", ( + "locale unpinned: English-text matching breaks off-locale" + ) From aaba5f7d23281f5af79a71437a919d8eff86ea23 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 13 Aug 2026 11:15:50 -0500 Subject: [PATCH 09/19] fix(journal): retain current next steps on resumed sessions --- src/synapt/recall/journal.py | 21 ++++++++++++++------- tests/recall/test_journal.py | 27 +++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/synapt/recall/journal.py b/src/synapt/recall/journal.py index 7b8f6122..d75ab26d 100644 --- a/src/synapt/recall/journal.py +++ b/src/synapt/recall/journal.py @@ -293,7 +293,8 @@ def read_previous_meaningful( def read_entries(path: Path | None = None, n: int = 5) -> list[JournalEntry]: """Read the last N journal entries (most recent first). - Deduplicates by session_id (keeps the richest entry per session) + Deduplicates by session_id (keeps the newest manual/auto entry; richness + breaks an exact-timestamp tie) and sorts by timestamp descending. Does NOT assume the file is chronologically ordered. """ @@ -324,18 +325,23 @@ def _read_all_entries(path: Path) -> list[JournalEntry]: def _entry_richness(entry: JournalEntry) -> tuple: """Score an entry for dedup ranking. - Returns a tuple that sorts higher for richer entries: - (not auto, rich field count, timestamp). + Manual entries still beat auto-extracted stubs, but within either class the + newest write wins before field count. A resumed runtime can reuse a session + id: letting an older, richer entry win there retains completed work and + hides the current entry's next steps, which are the continuity handoff. + + Returns a tuple that sorts higher for the retained entry: + (not auto, timestamp, rich field count). """ rich_count = sum(bool(f) for f in (entry.focus, entry.done, entry.decisions, entry.next_steps)) - return (not entry.auto, rich_count, entry.timestamp) + return (not entry.auto, entry.timestamp, rich_count) def _dedup_entries(entries: list[JournalEntry]) -> list[JournalEntry]: """Keep only the richest entry per session_id. - Priority: non-auto > auto, then most rich fields - (focus/done/decisions/next_steps), then newest timestamp. + Priority: non-auto > auto, then newest timestamp, then most rich fields + (focus/done/decisions/next_steps). Entries without a session_id are kept as-is. """ best: dict[str, JournalEntry] = {} @@ -353,7 +359,8 @@ def _dedup_entries(entries: list[JournalEntry]) -> list[JournalEntry]: def compact_journal(path: Path | None = None) -> int: """Physically dedup and sort journal.jsonl. - Reads all entries, deduplicates by session_id (keeps richest), + Reads all entries, deduplicates by session_id (keeps newest within the + manual/auto class, with richness as an exact-timestamp tie-breaker), sorts chronologically, and rewrites the file in-place. Uses an exclusive flock on the journal file itself (not a temp file) diff --git a/tests/recall/test_journal.py b/tests/recall/test_journal.py index cb8555cb..58dd3137 100644 --- a/tests/recall/test_journal.py +++ b/tests/recall/test_journal.py @@ -313,6 +313,29 @@ def test_read_entries_prefers_non_auto(self): self.assertEqual(entries[0].focus, "manual focus") self.assertFalse(entries[0].auto) + def test_read_entries_keeps_newer_current_next_steps_over_richer_prior_entry(self): + """A new session write must not lose its current next step to an older, + richer entry that happens to share a session id. + + Session IDs can be reused by a resumed runtime. The current entry is + the continuity handoff, so recency must win before richness: otherwise + a completed item remains visible and the next step just written drops. + """ + append_entry(JournalEntry( + timestamp="2026-08-13T10:00:00", session_id="resumed-session", + focus="prior work", done=["completed item"], decisions=["kept"], + next_steps=["prior unfinished"], auto=False, + ), self.path) + append_entry(JournalEntry( + timestamp="2026-08-13T11:00:00", session_id="resumed-session", + focus="current work", next_steps=["current next step"], auto=False, + ), self.path) + + latest = read_latest(self.path) + self.assertIsNotNone(latest) + self.assertEqual(latest.next_steps, ["current next step"]) + self.assertNotIn("completed item", latest.done) + def test_compact_journal_removes_duplicates(self): """compact_journal deduplicates and sorts the file.""" append_entry(JournalEntry( @@ -406,14 +429,14 @@ def test_compact_preserves_no_sid_entries(self): self.assertIn("A2", foci) def test_dedup_richness_tiebreaker_by_field_count(self): - """Between two auto entries for the same session, the one with more rich fields wins.""" + """When same-session writes tie on time, richer content breaks the tie.""" entries = [ JournalEntry( timestamp="2026-03-01T10:00:00", session_id="sess-A", focus="just focus", auto=True, ), JournalEntry( - timestamp="2026-03-01T11:00:00", session_id="sess-A", + timestamp="2026-03-01T10:00:00", session_id="sess-A", focus="focus + done", done=["task"], auto=True, ), ] From 8e89fa6c3f72e7a39a2a9cb31fb6dece6621d250 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Fri, 14 Aug 2026 16:04:30 -0500 Subject: [PATCH 10/19] feat(build): one signal for whether a build has anything to do `incremental` gates exactly one call site today, so a build with nothing to do still walks channels and journals and still pays for the walk. A real no-op needs a signal covering every input the build reads. Adds `build_delta`: a signature over archived transcripts, channel logs and journals, plus `is_noop` and manifest round-trip helpers. The dangerous direction here is "up to date", not "slow". A signal watching only transcripts would let a new channel message or a fresh journal entry go unindexed while the build reports success -- worse than a slow build, because the operator sees "nothing to do" and the thing they just wrote is missing from search with no error anywhere. Every input class is therefore in the signature and each has its own negative-control witness. Three properties the witnesses pin, each mutation-verified separately: - directories are globbed rather than read from a remembered list, so a newly arrived file changes the signature; - absence is never a no-op: no previous signature, a malformed one, or a version mismatch all resolve to doing the work, because the cost of a wrong None is one extra build and the cost of a wrong match is a build that never happens; - entries are sorted before hashing, so the digest depends on the input set and not on filesystem iteration order. Stated limit rather than a discovered one: content is not hashed, so an edit preserving both size and mtime is invisible to this signal. Hashing every byte on every build would cost more than the build this exists to skip. The archive layer refreshes on a newer mtime at equal size for the same reason. Wiring this into the build path lands in the next change of the series. Co-Authored-By: Claude Opus 5 --- src/synapt/recall/build_delta.py | 177 +++++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 src/synapt/recall/build_delta.py diff --git a/src/synapt/recall/build_delta.py b/src/synapt/recall/build_delta.py new file mode 100644 index 00000000..59a06ae7 --- /dev/null +++ b/src/synapt/recall/build_delta.py @@ -0,0 +1,177 @@ +"""Whether a build has anything to do, across every input class. + +``incremental`` today gates exactly one call site, so a build with nothing to +do still walks channels and journals and still pays for the walk. A real +no-op needs one signal that covers EVERY input the build reads: archived +transcripts, channel logs, and journals. + +*** THE DANGEROUS DIRECTION IS "UP TO DATE", NOT "SLOW". *** + +A signature that watches only transcripts would let a new channel message or a +fresh journal entry go unindexed while the build reports success. That is +worse than a slow build, because a slow build is visible and a silently +skipped one is not: the operator sees "nothing to do", and the thing they just +wrote is missing from search with no error anywhere. So every input class is +in the signature, and each has its own negative-control witness. + +WHAT THE SIGNATURE IS, AND ITS ONE HONEST LIMIT. Each input file contributes +its path, size, and modification time. Content is not hashed: on a store with +tens of thousands of archived turns, hashing every byte on every build costs +more than the build this module exists to skip. + +The residual, stated rather than left for someone to discover: an edit that +preserves BOTH size and mtime is invisible here. In practice a write moves +mtime, and the archive layer refreshes on a newer mtime at equal size for +exactly this reason. A deliberate same-size mtime-preserving rewrite is the +one case this signal misses, and a caller that needs to defeat it can pass +``--full``. It is written down because an unstated limit is indistinguishable +from a bug once somebody hits it. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path +from typing import Any, Iterable, Sequence +import hashlib + +#: Manifest key. Absent means "no previous build to compare against", which is +#: NOT the same as "nothing changed" -- see `signature_from_manifest`. +MANIFEST_KEY = "input_signature" + +SIGNATURE_VERSION = 1 + +_TRANSCRIPT_GLOB = "*.jsonl" + + +@dataclass(frozen=True) +class InputSignature: + """A fingerprint of every file a build would read. + + ``digest`` is what comparison uses. ``file_count`` rides along because a + signature that reports how much it saw makes an empty-inputs run legible + instead of looking identical to an unchanged one. + """ + + digest: str + file_count: int + + def to_manifest(self) -> dict[str, Any]: + """Wrapped under `MANIFEST_KEY`, so the reader can tell absent from empty. + + The wrapper is what makes `signature_from_manifest` able to answer + "this manifest predates signatures" distinctly from "this manifest has + a signature that happens to be malformed". A bare payload would make + every signature-less manifest indistinguishable from a corrupt one. + """ + return { + MANIFEST_KEY: { + "version": SIGNATURE_VERSION, + "digest": self.digest, + "files": self.file_count, + } + } + + +def _iter_input_files( + source_dirs: Sequence[Path] | None, + channels_dir: Path | None, + journal_paths: Iterable[Path] | None, +) -> list[Path]: + """Every file the build would read, from all three input classes. + + Directories are GLOBBED rather than read from a remembered list, so a newly + arrived file changes the signature. A signature built only from files it + already knew about cannot notice an arrival, which is the failure mode the + new-file witness pins. + """ + files: list[Path] = [] + + for directory in source_dirs or (): + directory = Path(directory) + if directory.is_dir(): + files.extend(p for p in directory.rglob(_TRANSCRIPT_GLOB) if p.is_file()) + + if channels_dir is not None: + channels_dir = Path(channels_dir) + if channels_dir.is_dir(): + files.extend(p for p in channels_dir.rglob(_TRANSCRIPT_GLOB) if p.is_file()) + + for journal in journal_paths or (): + journal = Path(journal) + if journal.is_file(): + files.append(journal) + + return files + + +def compute_input_signature( + source_dirs: Sequence[Path] | None = None, + channels_dir: Path | None = None, + journal_paths: Iterable[Path] | None = None, +) -> InputSignature: + """Fingerprint the build's inputs across transcripts, channels and journals. + + Sorted before hashing so the digest depends on the input set and not on + filesystem iteration order, which varies between runs and would otherwise + make two identical states compare as different -- a no-op signal that + reports "changed" on an unchanged store is merely useless, but one that + does so *intermittently* is worse, because it trains people to distrust it. + """ + entries: list[str] = [] + for path in _iter_input_files(source_dirs, channels_dir, journal_paths): + try: + stat = path.stat() + except OSError: + # Unreadable now, readable later, or vice versa: either way the + # input set is not what it was. Record the path so the change is + # visible rather than silently dropping the file from the digest. + entries.append(f"{path}\x00unreadable") + continue + entries.append(f"{path}\x00{stat.st_size}\x00{stat.st_mtime_ns}") + + entries.sort() + digest = hashlib.sha256("\x01".join(entries).encode("utf-8")).hexdigest() + return InputSignature(digest=digest, file_count=len(entries)) + + +def is_noop(previous: InputSignature | None, current: InputSignature) -> bool: + """True only when a prior signature exists and matches the current one. + + ABSENCE IS NEVER A NO-OP. A first build, or a build whose manifest was + lost or corrupt, has nothing to compare against -- and "I cannot tell" + must resolve to doing the work, not to skipping it. Treating an absent + previous as a match would make the very first build of a store, the one + that has the most to do, the one that does nothing. + """ + if previous is None: + return False + return previous.digest == current.digest + + +def signature_to_manifest(signature: InputSignature) -> dict[str, Any]: + """Render a signature for the manifest, JSON-round-trippable by construction.""" + return signature.to_manifest() + + +def signature_from_manifest(manifest: dict[str, Any] | None) -> InputSignature | None: + """Recover a signature, or None when there is nothing trustworthy to recover. + + Returns None for absent, malformed, or version-mismatched payloads. All + three mean the same thing to the caller -- no usable prior state -- and + `is_noop` turns that into "do the work". Failing toward work is the only + safe direction: the cost of a wrong None is one unnecessary build, and the + cost of a wrong signature is a build that never happens. + """ + if not isinstance(manifest, dict): + return None + payload = manifest.get(MANIFEST_KEY) + if not isinstance(payload, dict): + return None + if payload.get("version") != SIGNATURE_VERSION: + return None + digest = payload.get("digest") + files = payload.get("files") + if not isinstance(digest, str) or not digest or not isinstance(files, int): + return None + return InputSignature(digest=digest, file_count=files) From 3ece52e80a99c6b812393c014ceb89226019f23d Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Sat, 15 Aug 2026 05:04:18 -0500 Subject: [PATCH 11/19] fix(build-delta): validate the signature payload by shape, not by approximation signature_from_manifest promised in its own docstring to return None for malformed payloads and did not. It accepted six: files=True, files=False bool subclasses int, so isinstance admits both and the dataclass field declared int held True files=-1 nothing bounded the sign digest= "x" qualified as a sha256 digest version=True, version=1.0 found by closing the class rather than the three reported instances: the guard is a bare != and True == 1 and 1.0 == 1 Closes the class: version and files must be exactly int (type(...) is int, never isinstance, with the reason commented at the helper), files must be >= 0, and digest must match a fully anchored 64-lowercase-hex pattern. Any violation returns None, which is_noop turns into work. Scope stated honestly rather than dramatically: no WRONG NO-OP was reachable through these fields. is_noop compares digests, and a malformed digest cannot equal a real one, so a stale or garbage payload already resolved to work. What was reachable is a validator certifying "well-formed" over garbage, and a declared int holding a bool. A module whose entire argument is that untrustworthy state must resolve to work does not get to carry a gate that certifies what it did not check. Fourteen witnesses. Thirteen malformed payloads, each asserting BOTH halves -- the parse refuses AND is_noop turns the refusal into False -- because a guard returning None while a caller read None as "unchanged" would satisfy the first assertion and lose the property it exists to protect. The fourteenth is the acceptance control: thirteen tests asserting None would all pass against a validator that rejects everything, which fails in the direction that looks safe. Mutation-verified per guard, not once for the set, because stacked asserts short-circuit and one mutation would only prove the first. Reverting the files guard reddens exactly True/False/-1; reverting the digest guard reddens exactly the six digest shapes; dropping the version guard reddens exactly the two version cases. The acceptance control stays green under all three. --- src/synapt/recall/build_delta.py | 43 +++++++++++++- tests/recall/test_build_idempotence.py | 77 ++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 1 deletion(-) diff --git a/src/synapt/recall/build_delta.py b/src/synapt/recall/build_delta.py index 59a06ae7..c7fb7a4b 100644 --- a/src/synapt/recall/build_delta.py +++ b/src/synapt/recall/build_delta.py @@ -34,6 +34,7 @@ from pathlib import Path from typing import Any, Iterable, Sequence import hashlib +import re #: Manifest key. Absent means "no previous build to compare against", which is #: NOT the same as "nothing changed" -- see `signature_from_manifest`. @@ -43,6 +44,26 @@ _TRANSCRIPT_GLOB = "*.jsonl" +#: What `hashlib.sha256().hexdigest()` produces, and nothing else. Anchored at +#: both ends so a 64-hex substring inside a longer string does not qualify. +_HEX64 = re.compile(r"\A[0-9a-f]{64}\Z") + + +def _is_plain_int(value: Any) -> bool: + """`type(...) is int`, deliberately NOT `isinstance`. + + ``bool`` SUBCLASSES ``int`` in Python, so ``isinstance(True, int)`` is True + and ``True == 1``. An ``isinstance`` guard therefore admits ``True`` and + ``False`` into a field the dataclass declares as ``int``, and the same + equality makes ``version: True`` compare equal to ``SIGNATURE_VERSION``. + ``1.0 == 1`` gets in the same way. + + This is the whole class, not the one field it was first noticed on: any + numeric guard written with ``isinstance`` or bare ``==`` in this module has + the same hole. + """ + return type(value) is int + @dataclass(frozen=True) class InputSignature: @@ -162,16 +183,36 @@ def signature_from_manifest(manifest: dict[str, Any] | None) -> InputSignature | `is_noop` turns that into "do the work". Failing toward work is the only safe direction: the cost of a wrong None is one unnecessary build, and the cost of a wrong signature is a build that never happens. + + THIS DOCSTRING USED TO OVERSTATE WHAT THE CODE DID, which is the reason the + validation below is shape-exact rather than approximate. The first version + accepted six malformed payloads while promising to reject them: `files` as + ``True``/``False``/``-1`` (``bool`` subclasses ``int``, and nothing bounded + the sign), `digest` as any non-empty string, and -- found while closing the + class rather than the reported instances -- `version` as ``True`` or ``1.0``, + both of which compare equal to ``1``. + + Stated exactly, because the honest scope matters more than the scary one: no + WRONG NO-OP was reachable through those fields, since `is_noop` compares + digests and a malformed digest cannot equal a real one. What was reachable + is worse in a quieter way -- a validator reporting "well-formed" over + garbage, and a dataclass whose declared ``int`` could hold ``True``. A gate + that certifies what it did not check is the defect this module exists to + argue against, so it does not get to have one. """ if not isinstance(manifest, dict): return None payload = manifest.get(MANIFEST_KEY) if not isinstance(payload, dict): return None + if not _is_plain_int(payload.get("version")): + return None if payload.get("version") != SIGNATURE_VERSION: return None digest = payload.get("digest") files = payload.get("files") - if not isinstance(digest, str) or not digest or not isinstance(files, int): + if not isinstance(digest, str) or _HEX64.match(digest) is None: + return None + if not _is_plain_int(files) or files < 0: return None return InputSignature(digest=digest, file_count=files) diff --git a/tests/recall/test_build_idempotence.py b/tests/recall/test_build_idempotence.py index 7aef5b25..54805dba 100644 --- a/tests/recall/test_build_idempotence.py +++ b/tests/recall/test_build_idempotence.py @@ -364,6 +364,83 @@ def test_signature_absent_from_manifest_returns_none(tmp_path): assert signature_from_manifest({"source_files": []}) is None +@pytest.mark.parametrize( + "label,payload", + [ + # `bool` subclasses `int`, so an isinstance guard admits both, and the + # dataclass field declared `int` ends up holding True. + ("files is True", {"version": 1, "digest": "a" * 64, "files": True}), + ("files is False", {"version": 1, "digest": "a" * 64, "files": False}), + # Nothing bounded the sign. A negative count is not a count. + ("files is -1", {"version": 1, "digest": "a" * 64, "files": -1}), + ("files is a float", {"version": 1, "digest": "a" * 64, "files": 3.0}), + ("files is a digit string", {"version": 1, "digest": "a" * 64, "files": "3"}), + # A digest is what sha256().hexdigest() produces, not any string. + ("digest is too short", {"version": 1, "digest": "x", "files": 3}), + ("digest is non-hex", {"version": 1, "digest": "z" * 64, "files": 3}), + ("digest is uppercase", {"version": 1, "digest": "A" * 64, "files": 3}), + ("digest is 63 chars", {"version": 1, "digest": "a" * 63, "files": 3}), + ("digest is 65 chars", {"version": 1, "digest": "a" * 65, "files": 3}), + ("digest is padded", {"version": 1, "digest": " " + "a" * 64, "files": 3}), + # Found by closing the CLASS rather than the three reported instances: + # the version guard is a bare `!=`, and True == 1 and 1.0 == 1. + ("version is True", {"version": True, "digest": "a" * 64, "files": 3}), + ("version is 1.0", {"version": 1.0, "digest": "a" * 64, "files": 3}), + ], +) +def test_a_malformed_signature_payload_resolves_to_WORK(label, payload): + """Rejecting is only half of it: the refusal must reach a BUILD. + + `signature_from_manifest` returning None is an internal fact. What the + module promises is that no usable prior state means the work happens, so + each case asserts BOTH halves -- the parse refuses, AND `is_noop` turns + that refusal into False. A guard that returned None while some caller + treated None as "unchanged" would satisfy the first assertion and lose the + property the first assertion exists to protect. + + Written after the original validator accepted every payload here while its + own docstring said it rejected malformed ones. + """ + from synapt.recall.build_delta import ( + InputSignature, + is_noop, + signature_from_manifest, + ) + + recovered = signature_from_manifest({"input_signature": payload}) + assert recovered is None, f"malformed payload was accepted: {label}" + + current = InputSignature(digest="b" * 64, file_count=5) + assert is_noop(recovered, current) is False, ( + f"refusal did not resolve to work: {label}" + ) + + +def test_a_well_formed_payload_is_still_accepted(): + """The control for the rejection cases above. + + Thirteen tests asserting None would all pass against a validator that + rejects everything, including real signatures -- a guard that refuses + universally is exactly as broken as one that accepts universally, and it + fails in the direction that looks safe. This is the case that would go red + if the new checks were tightened past correctness. + """ + from synapt.recall.build_delta import ( + InputSignature, + is_noop, + signature_from_manifest, + ) + + good = {"version": 1, "digest": "a" * 64, "files": 3} + recovered = signature_from_manifest({"input_signature": good}) + + assert recovered is not None, "a well-formed payload must survive" + assert recovered.digest == "a" * 64 + assert recovered.file_count == 3 + assert type(recovered.file_count) is int + assert is_noop(recovered, InputSignature(digest="a" * 64, file_count=3)) is True + + # =========================================================================== # D. The build must never grind LLM summaries # =========================================================================== From 68985ad97d27c5d5067dda7bb51ceee7e7f243ae Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Sat, 15 Aug 2026 05:32:20 -0500 Subject: [PATCH 12/19] refactor(build): stop running the summary grinder from a build `_archive_and_build` called `upgrade_large_cluster_summaries` on every build. That is unbounded work of a different KIND from everything else in the build path: the rest is local and scales with what changed, while this makes LLM calls and ran regardless. It also sat inside a bare `except: pass`, so a build reported success whether the grinder worked, did nothing, or failed -- the state that makes a cost invisible. Its explicit home is the `maintain` subcommand, which lands later in this series. WHAT IS AND IS NOT LOST MEANWHILE. Summaries come in TWO TIERS, and every loose sentence about this change has been wrong by collapsing them. CONCAT is the baseline and is unaffected: the clustering step earlier in the same function pre-generates a concat summary for EVERY cluster on every build, skipping only those that already hold an LLM one. No cluster is left without a summary by this change. LLM is the upgrade and has two triggers. `process_build_promotions` upgrades by ACCESS TIER and still runs. The removed pass upgraded by SIZE, independent of access -- which is why its query looked for clusters where `method = 'llm'` was absent. So the residual is exactly this: a cluster that is large but rarely searched keeps its concat summary and waits for `maintain` to be UPGRADED to LLM quality. It does not go unsummarized. Two earlier drafts said "summaries stop being generated" and then "no longer gets a summary". Same error twice -- writing "summary" unqualified in a system that has two tiers of them -- and the second was caught in review rather than by me. Satisfies the merged spec `test_build_never_calls_the_summary_grinder`. Mutation-verified: restoring the call turns that witness red. Full suite 11 failed / 2860 passed; every failure is a later-group TDD spec in the same file (maintain, incremental-default, archive-refresh), down from 12. Nothing outside the series changed, and nothing asserted on the removed print. --- src/synapt/recall/cli.py | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/src/synapt/recall/cli.py b/src/synapt/recall/cli.py index 821a4c92..d17cf2fb 100644 --- a/src/synapt/recall/cli.py +++ b/src/synapt/recall/cli.py @@ -561,14 +561,38 @@ def _archive_and_build_locked( except Exception: pass # Never fail a build due to promotions - # Upgrade large clusters to LLM summaries (size-based, not access-based) - try: - from synapt.recall.clustering import upgrade_large_cluster_summaries - llm_upgraded = upgrade_large_cluster_summaries(db, min_chunks=5, max_upgrades=5) - if llm_upgraded: - print(f" LLM summaries: {llm_upgraded} clusters upgraded") - except Exception: - pass # Never fail a build due to LLM summaries + # THE SUMMARY GRINDER IS NOT RUN FROM A BUILD. + # + # `upgrade_large_cluster_summaries` makes LLM calls, so it is unbounded work + # of a different KIND from everything else here: the rest of a build is + # local and its cost scales with what changed, while this scales with an + # external service and runs on every build regardless. It also sat inside a + # bare `except: pass`, so a build reported success whether it worked, did + # nothing, or failed -- which is the state that makes a cost invisible. + # + # Its explicit home is the `maintain` subcommand, which does not exist yet. + # + # WHAT IS AND IS NOT LOST IN THE MEANTIME. Summaries come in TWO TIERS, and + # every loose sentence about this change has been wrong by collapsing them: + # + # CONCAT is the baseline, and it is unaffected. The clustering step above + # pre-generates a concat summary for EVERY cluster on every build, skipping + # only those that already hold an LLM one. So no cluster is left without a + # summary by this change. + # + # LLM is the upgrade, and it has two triggers. `process_build_promotions` + # upgrades by ACCESS TIER and still runs. The removed pass upgraded by + # SIZE, independent of access -- which is why its query looked for + # clusters where `method = 'llm'` was absent. + # + # So the residual is exactly this: a cluster that is large but rarely + # searched keeps its concat summary and waits for `maintain` to be UPGRADED + # to LLM quality. It does not go unsummarized. + # + # Recorded at this length because two earlier drafts of this comment said + # "summaries stop being generated" and then "no longer gets a summary" -- + # the same error twice, from writing "summary" unqualified in a system that + # has two tiers of them. # Maintain adaptive memory: decay, archival, log compaction try: From 7d474877419ad1806386471d5d997a19cfb870f6 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Sat, 15 Aug 2026 06:14:24 -0500 Subject: [PATCH 13/19] fix(build): scope the parse-skip key by source dir, and refresh on mtime Two independent staleness defects, both of which made an incremental build skip a file that had genuinely changed, silently and permanently. Parse-skip keying. `source_files` is one flat list spanning every build source, while the reader keyed `already_indexed` on the BASENAME alone. Two worktrees archiving the same session name produce two entries; under a basename key the second overwrites the first, so the loser's stamp can never match its own file and it re-parses on every incremental build, forever. The key is now (source dir, name), written on both sides -- a reader-only change would have passed its test and stayed inert in production, because manifests never carried the dir. Manifests written before the field keep the old flat key so an upgrade does not force a full rebuild. Archive freshness. `archive_transcripts` skipped whenever the source and the archived copy had equal size, treating equal length as equal content. An edit that preserves byte count is invisible to that check, so it was never re-archived and therefore never re-indexed -- and the docstring asserted the behaviour as intended. mtime is now a second, independent signal, compared in ns so float rounding cannot make an unchanged file look newer than itself. Both negative controls hold: unchanged files stay a no-op, and a shrunken source still does not overwrite a longer archive. Also pins three properties that were correct but unwitnessed, so a later change cannot weaken them silently: - a digest with a trailing newline is rejected, which discriminates the `\Z` anchor from `$` -- `$` matches before a trailing newline, and the suite could not previously tell the two apart - a zero-file signature is accepted, bounding the guard from the side the rejection cases do not reach - is_noop compares the digest and deliberately not the file count, which is derived from the same input rather than independent evidence about it Each new witness is killed by its own mutation and by no other. --- src/synapt/recall/archive.py | 37 ++++++++++++--- src/synapt/recall/build_delta.py | 12 +++++ src/synapt/recall/cli.py | 5 ++ src/synapt/recall/core.py | 32 +++++++++++-- tests/recall/test_build_idempotence.py | 63 ++++++++++++++++++++++++++ 5 files changed, 138 insertions(+), 11 deletions(-) diff --git a/src/synapt/recall/archive.py b/src/synapt/recall/archive.py index c5f2d75a..d3ca75cd 100644 --- a/src/synapt/recall/archive.py +++ b/src/synapt/recall/archive.py @@ -681,9 +681,10 @@ def save_sync_config(project_dir: Path, config: dict) -> Path: def archive_transcripts(project_dir: Path, source_dir: Path) -> list[Path]: """Copy new transcript files from Claude Code's source dir to the project archive. - Skips files that already exist with the same size. Overwrites if the - source grew since last archive. Preserves larger archives when the - source shrinks (e.g., /clear truncated the transcript). + Skips files that already exist with the same size AND no newer mtime. + Overwrites if the source grew since last archive, or if it was modified + without changing length. Preserves larger archives when the source + shrinks (e.g., /clear truncated the transcript). Args: project_dir: Root of the project (where .synapt/recall/ lives). @@ -698,12 +699,34 @@ def archive_transcripts(project_dir: Path, source_dir: Path) -> list[Path]: copied = [] for src_file in sorted(source_dir.glob("*.jsonl")): dst_file = archive_dir / src_file.name - src_size = src_file.stat().st_size + src_stat = src_file.stat() + src_size = src_stat.st_size if dst_file.exists(): - dst_size = dst_file.stat().st_size + dst_stat = dst_file.stat() + dst_size = dst_stat.st_size if src_size == dst_size: - continue # No change - if src_size < dst_size: + # Equal size is NOT equal content. An edit that preserves the + # byte count leaves the size identical, so size alone can never + # observe it and the file is skipped on every subsequent build, + # permanently. mtime is the second, independent signal; compare + # it in ns so float rounding cannot make an unchanged file look + # newer than itself. copy2 replicates the source mtime as + # faithfully as the DESTINATION filesystem allows, which is + # exactly where this can degrade: the archive may sit on a + # different filesystem than the source, and one storing a + # coarser mtime truncates the copied stamp, so the destination + # reads as older than its own source and the file re-copies on + # every build. + # + # That degeneration is accepted deliberately, because the two + # ways of being wrong are not symmetric. A strict comparison + # errs toward WORK: the cost is a bounded, repeated copy. A + # tolerance would err toward SKIP, silently treating an edit + # inside the tolerance window as unchanged -- which is the + # exact defect class this check exists to kill. + if src_stat.st_mtime_ns <= dst_stat.st_mtime_ns: + continue + elif src_size < dst_size: continue # Source shrunk (e.g., /clear truncated) — keep larger archive shutil.copy2(src_file, dst_file) copied.append(dst_file) diff --git a/src/synapt/recall/build_delta.py b/src/synapt/recall/build_delta.py index c7fb7a4b..059a4f22 100644 --- a/src/synapt/recall/build_delta.py +++ b/src/synapt/recall/build_delta.py @@ -164,6 +164,18 @@ def is_noop(previous: InputSignature | None, current: InputSignature) -> bool: must resolve to doing the work, not to skipping it. Treating an absent previous as a match would make the very first build of a store, the one that has the most to do, the one that does nothing. + + THE DIGEST IS THE ONLY THING COMPARED, and `file_count` is deliberately + not. The digest is computed over every entry, so the count is derived + from the same input rather than independent evidence about it: any change + that alters the count necessarily alters the digest. Comparing it too + would look like defence in depth and provide none, because a second check + over the same input cannot fail when the first one passes. + + That makes this a real contract rather than an implementation detail. If + the digest ever stops covering the full entry set, this function silently + weakens, and nothing here would notice -- so the assumption is pinned by + a witness rather than left to this comment. """ if previous is None: return False diff --git a/src/synapt/recall/cli.py b/src/synapt/recall/cli.py index d17cf2fb..c7d9e9f5 100644 --- a/src/synapt/recall/cli.py +++ b/src/synapt/recall/cli.py @@ -643,6 +643,11 @@ def _archive_and_build_locked( st = fp.stat() source_files.append({ "name": fp.name, + # The source dir scopes the name. This list is FLAT across every + # build source, so two worktrees archiving the same session name + # produce two entries that are indistinguishable without it, and + # a basename-keyed reader silently keeps only one of them. + "dir": build_source.name, "mtime": st.st_mtime, "size": st.st_size, }) diff --git a/src/synapt/recall/core.py b/src/synapt/recall/core.py index 556bf4d9..29b520e0 100644 --- a/src/synapt/recall/core.py +++ b/src/synapt/recall/core.py @@ -4911,10 +4911,31 @@ def build_index( pass # Filter for incremental builds — skip files whose mtime AND size match - already_indexed: dict[str, tuple[float, int]] = {} + # Keyed on (source dir, name), never the basename alone: `source_files` is + # ONE flat list spanning every build source, so two worktrees archiving the + # same session name yield two entries. Under a basename key the second + # overwrites the first, and the loser's stamp can never match its own file — + # so it re-parses on every incremental build, forever, with no error. + # + # The key uses the source dir's BASENAME, which is unique only because the + # build sources are siblings under one root. A future multi-root source + # list can collide here the same way basenames collide today, and would + # need the fuller path. Impossible by topology now; stated so that whoever + # adds the second root meets this instead of rediscovering it. + already_indexed: dict[tuple[str, str], tuple[float, int]] = {} + # Manifests written before entries carried "dir" keep the old flat key. + # They cannot distinguish colliding basenames — that is the defect — but a + # legacy manifest is better than forcing one full rebuild on upgrade, and + # the next build rewrites the manifest in the scoped form. + legacy_indexed: dict[str, tuple[float, int]] = {} if incremental_manifest: for src in incremental_manifest.get("source_files", []): - already_indexed[src["name"]] = (src.get("mtime", 0), src.get("size", 0)) + stamp = (src.get("mtime", 0), src.get("size", 0)) + src_dir = src.get("dir") + if src_dir is None: + legacy_indexed[src["name"]] = stamp + else: + already_indexed[(src_dir, src["name"])] = stamp from synapt.recall.codex import is_codex_transcript, parse_codex_transcript @@ -4924,8 +4945,11 @@ def build_index( parsed_files: list[Path] = [] # Track which files were actually parsed for filepath in jsonl_files: - if filepath.name in already_indexed: - stored_mtime, stored_size = already_indexed[filepath.name] + stamp = already_indexed.get((filepath.parent.name, filepath.name)) + if stamp is None: + stamp = legacy_indexed.get(filepath.name) + if stamp is not None: + stored_mtime, stored_size = stamp stat = filepath.stat() if stat.st_mtime == stored_mtime and stat.st_size == stored_size: skipped += 1 diff --git a/tests/recall/test_build_idempotence.py b/tests/recall/test_build_idempotence.py index 54805dba..6bf5b177 100644 --- a/tests/recall/test_build_idempotence.py +++ b/tests/recall/test_build_idempotence.py @@ -382,6 +382,12 @@ def test_signature_absent_from_manifest_returns_none(tmp_path): ("digest is 63 chars", {"version": 1, "digest": "a" * 63, "files": 3}), ("digest is 65 chars", {"version": 1, "digest": "a" * 65, "files": 3}), ("digest is padded", {"version": 1, "digest": " " + "a" * 64, "files": 3}), + # Pins `\Z` against `$`, which are NOT interchangeable: `$` matches + # before a trailing newline, so a `$`-anchored pattern accepts this + # payload and the digest silently carries the newline into every later + # comparison. The suite could not discriminate the two anchors before + # this case, so the correct anchor was correct but unpinned. + ("digest has a trailing newline", {"version": 1, "digest": "a" * 64 + "\n", "files": 3}), # Found by closing the CLASS rather than the three reported instances: # the version guard is a bare `!=`, and True == 1 and 1.0 == 1. ("version is True", {"version": True, "digest": "a" * 64, "files": 3}), @@ -441,6 +447,63 @@ def test_a_well_formed_payload_is_still_accepted(): assert is_noop(recovered, InputSignature(digest="a" * 64, file_count=3)) is True +def test_a_zero_file_signature_is_ACCEPTED(): + """Widens the acceptance control, which previously proved one shape only. + + `files: 0` sits directly against the `files < 0` boundary and is a real + state -- a store whose sources are all gone still has a signature, and a + build over zero files is a legitimate no-op rather than a corrupt manifest. + Nothing pinned it, so a tightening of `< 0` into `<= 0` would have been + rejected by no test while looking like a stricter, safer guard. + + The rejection cases bound the guard from one side. Without this, the only + acceptance evidence was a single mid-range payload, and a guard that + admitted exactly that one shape would have passed the entire suite. + """ + from synapt.recall.build_delta import signature_from_manifest + + recovered = signature_from_manifest( + {"input_signature": {"version": 1, "digest": "c" * 64, "files": 0}} + ) + + assert recovered is not None, "a zero-file signature is well-formed, not malformed" + assert recovered.file_count == 0 + assert type(recovered.file_count) is int + + +def test_is_noop_compares_the_DIGEST_and_not_the_file_count(): + """Pins the contract `is_noop`'s docstring states, so it cannot drift silently. + + The digest covers every entry, so `file_count` is derived from the same + input rather than independent evidence about it -- comparing both would be + defence in depth that provides none. This witness makes that a checked + claim instead of a comment: same digest, deliberately mismatched counts, + still a no-op. + + It is written to fail in BOTH directions. Adding a `file_count` comparison + turns the first assertion red; weakening the digest comparison turns the + second one red. A future reader who "fixes" this function by making it + stricter now has to argue with a test rather than a paragraph. + """ + from synapt.recall.build_delta import InputSignature, is_noop + + same_digest_different_count = is_noop( + InputSignature(digest="d" * 64, file_count=3), + InputSignature(digest="d" * 64, file_count=99), + ) + assert same_digest_different_count is True, ( + "file_count was compared; the digest is meant to be authoritative" + ) + + different_digest_same_count = is_noop( + InputSignature(digest="d" * 64, file_count=3), + InputSignature(digest="e" * 64, file_count=3), + ) + assert different_digest_same_count is False, ( + "digests differed and it still claimed a no-op" + ) + + # =========================================================================== # D. The build must never grind LLM summaries # =========================================================================== From 7eef451817d09c59a6338d412d7fcb3364a5ed08 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Tue, 18 Aug 2026 23:30:47 -0500 Subject: [PATCH 14/19] fix(recall): export/import resolve the recall root, never the cwd The archive verbs computed Path.cwd() and passed it as the workspace root. project_data_dir consults SYNAPT_RECALL_ROOT only when no root is passed, so forwarding cwd did not merely skip the override, it suppressed it -- the same env var honored by stats and journal was silently ignored by export and import, and an export run from a checkout carried the entire live corpus instead of the workspace the operator named. cmd_import carried the identical line. One seam, four callers. archive.py gains _resolve_project_dir, which routes through project_data_dir(None) so the env override, git-worktree inference, and gripspace inference apply identically to the archive verbs and to every other recall surface. It verifies the .synapt/recall suffix before stripping it, the same defense channel.py already uses, rather than a blind parents[1]. export_recall_archive and import_recall_archive accept None and resolve there. The CLI and MCP callers stop pretending to know the root. The CLI gains --path for the case where a caller genuinely does. Both verbs now report the RESOLVED store they touched, recorded after inference, never the caller's argument. Export writes data_dir into the manifest -- the directory the bytes came from -- and prints it. Import returns data_dir in its summary -- the destination the bytes landed in -- and prints it, while the archive's own origin store travels as source_data_dir so the two are never conflated: echoing the manifest's field would name where the bytes came from and read like a destination. A bare chunk count with no path is exactly the report shape that let a wrong-store export hide; an argument-derived path would let an inference-redirected --path hide the same way. Six witnesses. Export and import against the env override, two roots each, both mutation-killed by reverting the resolver to cwd. Export reporting against a real linked worktree with a liveness control, mutation-killed by recording the argument instead of the resolved dir. Import reporting from a source root, under an override root, standing in a third, driving both the CLI print and the MCP text, mutation-killed by echoing the archive's source store. Absent provenance in both import modes, against a real pre-provenance archive rewritten from a current one, with the current archive as the control: the key is omitted rather than None, mutation-killed by dropping the guard. Same-shell fruit: a configuration that previously exported the entire live corpus now exports the one planted entry and names the store it read. Co-Authored-By: Claude --- src/synapt/recall/archive.py | 66 ++++++- src/synapt/recall/cli.py | 16 +- src/synapt/recall/server.py | 12 +- tests/recall/test_archive.py | 329 +++++++++++++++++++++++++++++++++++ 4 files changed, 411 insertions(+), 12 deletions(-) diff --git a/src/synapt/recall/archive.py b/src/synapt/recall/archive.py index d3ca75cd..72db6567 100644 --- a/src/synapt/recall/archive.py +++ b/src/synapt/recall/archive.py @@ -44,6 +44,27 @@ def _data_dir(project_dir: Path) -> Path: return project_data_dir(project_dir) +def _resolve_project_dir(project_dir: Path | None) -> Path: + """Resolve the workspace root, honoring ``SYNAPT_RECALL_ROOT`` when unset. + + ``project_data_dir`` returns ``/.synapt/recall``; the archive code + wants ````. Derive it from the resolved data dir so the env override, + git-worktree, and gripspace inference all apply identically to the archive + verbs and to every other recall surface -- and verify the expected suffix + before stripping it (the same defense ``channel.py`` uses) rather than + trusting a blind ``parents[1]``. + """ + if project_dir is not None: + return Path(project_dir).resolve() + data_dir = project_data_dir(None) + if data_dir.parent.name == ".synapt" and data_dir.name == "recall": + return data_dir.parent.parent + raise RuntimeError( + f"resolved recall data dir has an unexpected shape: {data_dir} " + f"(expected /.synapt/recall); refusing to guess the root" + ) + + def _index_dir(project_dir: Path) -> Path: """Return the recall index directory for *project_dir*.""" return _data_dir(project_dir) / "index" @@ -163,6 +184,13 @@ def _archive_manifest( "version": ARCHIVE_FORMAT_VERSION, "synapt_version": synapt_version, "created_at": datetime.now(timezone.utc).isoformat(), + # The RESOLVED data directory the bytes actually came from, recorded + # after git-worktree and gripspace inference -- not the caller's + # argument. Reported by the CLI and MCP surfaces so a wrong-store + # export cannot hide behind plausible-looking counts, and so an + # explicit --path that inference redirects cannot hide behind a + # plausible-looking path either (recall#963 naming contract). + "data_dir": str(data_dir), "source_project": project_dir.name, "chunk_count": chunk_count, "knowledge_count": knowledge_count, @@ -199,14 +227,23 @@ def _tar_add_file(tf: tarfile.TarFile, path: Path, arcname: str) -> None: def export_recall_archive( - project_dir: Path, + project_dir: Path | None = None, output_path: Path | None = None, *, exclude_transcripts: bool = False, exclude_channels: bool = False, ) -> tuple[Path, dict]: - """Export portable recall state to a ``.synapt-archive`` tar.gz file.""" - project_dir = project_dir.resolve() + """Export portable recall state to a ``.synapt-archive`` tar.gz file. + + *project_dir* is the workspace root to export. Pass ``None`` to resolve it + the same way every other recall surface does -- ``SYNAPT_RECALL_ROOT`` + first, then git/gripspace inference, then cwd. Callers should only pass an + explicit root when they genuinely know it: ``project_data_dir`` consults + the env override ONLY when no root is passed, so a caller that forwards + ``Path.cwd()`` does not merely skip the override, it suppresses it. That + is how ``recall export`` silently exported the wrong store. + """ + project_dir = _resolve_project_dir(project_dir) data_dir = _data_dir(project_dir) if not data_dir.exists(): raise FileNotFoundError(f"No recall data found at {data_dir}") @@ -534,7 +571,7 @@ def _rebuild_merged_index( def import_recall_archive( - project_dir: Path, + project_dir: Path | None, archive_path: Path, *, mode: str = "replace", @@ -544,7 +581,20 @@ def import_recall_archive( ``mode="replace"`` fully restores the archived data directory. ``mode="merge"`` merges transcripts, journals, channels, reminders, and reconstructs a merged monolithic recall index from both sources. + + *project_dir* follows the same rule as :func:`export_recall_archive`: + ``None`` resolves via ``SYNAPT_RECALL_ROOT`` and inference; an explicit + root suppresses the override, so pass one only when you truly know it. + + The returned summary's ``data_dir`` is the RESOLVED destination store + this import wrote into, recorded after inference. The archive's own + origin store (its manifest ``data_dir``) is carried as + ``source_data_dir`` so the two are never confused: one names where the + bytes came from, the other where they landed. Archives written before + the manifest carried ``data_dir`` have no provenance to report, and the + key is omitted rather than set to ``None``, so absent reads as absent. """ + project_dir = _resolve_project_dir(project_dir) if mode not in {"replace", "merge"}: raise ValueError("mode must be 'replace' or 'merge'") @@ -573,6 +623,10 @@ def import_recall_archive( data_dir.parent.mkdir(parents=True, exist_ok=True) shutil.copytree(extracted_dir, data_dir) summary = dict(manifest) + source_store = summary.pop("data_dir", None) + if source_store is not None: + summary["source_data_dir"] = source_store + summary["data_dir"] = str(data_dir) summary["mode"] = "replace" return summary @@ -632,6 +686,10 @@ def import_recall_archive( summary = dict(manifest) summary.update(merged_index_stats) + source_store = summary.pop("data_dir", None) + if source_store is not None: + summary["source_data_dir"] = source_store + summary["data_dir"] = str(data_dir) summary["mode"] = "merge" summary["session_count"] = len({c.session_id for c in merged_chunks}) return summary diff --git a/src/synapt/recall/cli.py b/src/synapt/recall/cli.py index c7d9e9f5..5e9933dc 100644 --- a/src/synapt/recall/cli.py +++ b/src/synapt/recall/cli.py @@ -1284,7 +1284,10 @@ def cmd_export(args: argparse.Namespace) -> None: """Export portable recall state to a .synapt-archive file.""" from synapt.recall.archive import export_recall_archive - project = Path.cwd().resolve() + # Do NOT default to Path.cwd() here: an explicit root suppresses the + # SYNAPT_RECALL_ROOT override inside project_data_dir. None means "resolve + # like every other recall verb". + project = Path(args.path).expanduser().resolve() if getattr(args, "path", None) else None try: output_path, manifest = export_recall_archive( project, @@ -1296,7 +1299,10 @@ def cmd_export(args: argparse.Namespace) -> None: print(f"Export failed: {exc}", file=sys.stderr) sys.exit(1) + # Name the resolved store (recall#963 reporting contract): a bare count is + # exactly the report shape that let a wrong-store export hide. print(f"Exported recall archive to {output_path}") + print(f" store={manifest.get('data_dir', '?')}") print( f" chunks={manifest.get('chunk_count', 0)} " f"knowledge={manifest.get('knowledge_count', 0)} " @@ -1308,7 +1314,8 @@ def cmd_import(args: argparse.Namespace) -> None: """Import portable recall state from a .synapt-archive file.""" from synapt.recall.archive import import_recall_archive - project = Path.cwd().resolve() + # Same rule as export: None resolves via SYNAPT_RECALL_ROOT + inference. + project = Path(args.path).expanduser().resolve() if getattr(args, "path", None) else None mode = "merge" if args.merge else "replace" try: summary = import_recall_archive( @@ -1324,7 +1331,8 @@ def cmd_import(args: argparse.Namespace) -> None: print( f" mode={summary.get('mode', mode)} " f"chunks={summary.get('chunk_count', 0)} " - f"knowledge={summary.get('knowledge_count', 0)}" + f"knowledge={summary.get('knowledge_count', 0)} " + f"store={summary.get('data_dir', '?')}" ) @@ -2885,12 +2893,14 @@ def main(): export_parser.add_argument("output", nargs="?", default=None, help="Output .synapt-archive path (default: .synapt-archive)") export_parser.add_argument("--exclude-transcripts", action="store_true", help="Skip raw transcript archives") export_parser.add_argument("--exclude-channels", action="store_true", help="Skip channel history files") + export_parser.add_argument("--path", default=None, help="Workspace root to export (default: SYNAPT_RECALL_ROOT, else inferred from git/gripspace, else cwd)") import_parser = subparsers.add_parser("import", help="Import portable recall data from a .synapt-archive file") import_parser.add_argument("archive", help="Path to a .synapt-archive file") import_mode = import_parser.add_mutually_exclusive_group() import_mode.add_argument("--merge", action="store_true", help="Merge imported data into existing recall state") import_mode.add_argument("--replace", action="store_true", help="Replace existing recall state (default)") + import_parser.add_argument("--path", default=None, help="Workspace root to import into (default: SYNAPT_RECALL_ROOT, else inferred from git/gripspace, else cwd)") # Transcript (display/save a session) transcript_parser = subparsers.add_parser("transcript", help="Display or save a session transcript") diff --git a/src/synapt/recall/server.py b/src/synapt/recall/server.py index cd20332a..cc10e515 100644 --- a/src/synapt/recall/server.py +++ b/src/synapt/recall/server.py @@ -658,10 +658,11 @@ def recall_export( """ from synapt.recall.archive import export_recall_archive - project = Path.cwd().resolve() + # None => resolve via SYNAPT_RECALL_ROOT + inference. Forwarding Path.cwd() + # would suppress the override. try: archive_path, manifest = export_recall_archive( - project, + None, Path(output_path).expanduser() if output_path else None, exclude_transcripts=exclude_transcripts, exclude_channels=exclude_channels, @@ -671,6 +672,7 @@ def recall_export( return ( f"Recall archive exported to {archive_path}\n" + f" - store: {manifest.get('data_dir', '?')}\n" f" - chunks: {manifest.get('chunk_count', 0)}\n" f" - knowledge: {manifest.get('knowledge_count', 0)}\n" f" - worktrees: {manifest.get('worktree_count', 0)}" @@ -686,9 +688,8 @@ def recall_import(archive_path: str, mode: str = "replace") -> str: """ from synapt.recall.archive import import_recall_archive - project = Path.cwd().resolve() try: - summary = import_recall_archive(project, Path(archive_path), mode=mode) + summary = import_recall_archive(None, Path(archive_path), mode=mode) except Exception as e: return f"Import failed: {e}" @@ -696,7 +697,8 @@ def recall_import(archive_path: str, mode: str = "replace") -> str: f"Recall archive imported from {Path(archive_path).expanduser().resolve()}\n" f" - mode: {summary.get('mode', mode)}\n" f" - chunks: {summary.get('chunk_count', 0)}\n" - f" - knowledge: {summary.get('knowledge_count', 0)}" + f" - knowledge: {summary.get('knowledge_count', 0)}\n" + f" - store: {summary.get('data_dir', '?')}" ) diff --git a/tests/recall/test_archive.py b/tests/recall/test_archive.py index 896d85d3..ccc42958 100644 --- a/tests/recall/test_archive.py +++ b/tests/recall/test_archive.py @@ -1,10 +1,13 @@ """Tests for transcript archiving and sync configuration.""" +import argparse +import pytest import json from pathlib import Path from unittest.mock import patch, MagicMock from synapt.recall.archive import ( + _load_archive_manifest, export_recall_archive, import_recall_archive, archive_transcripts, @@ -658,3 +661,329 @@ def test_should_sync_true_after_interval(tmp_path): ts_path.write_text(str(time_mod.time() - 900)) assert should_sync(project, min_interval_minutes=10) is True + + +# --- store resolution in the export/import CLI verbs ------------------------- + +def _archive_member_names(archive_path: Path) -> list[str]: + import tarfile + + with tarfile.open(archive_path, "r:gz") as tf: + return tf.getnames() + + +def test_export_resolves_the_recall_root_override_not_the_cwd(tmp_path, monkeypatch): + """`recall export` must export the store SYNAPT_RECALL_ROOT names. + + **Two roots, because one root cannot bind this.** With a single store, + "resolved the override" and "resolved the cwd" produce byte-identical + archives, so no assertion over that fixture can tell them apart. + + Mechanism under test: ``cmd_export`` computes ``Path.cwd().resolve()`` and + passes it to ``export_recall_archive`` -> ``project_data_dir(project_dir)``. + ``project_data_dir`` consults the env override ONLY when *project_dir* is + None, so passing an explicit cwd does not merely skip the override, it + actively suppresses it. + + It fails SILENTLY: an operator exporting what they believe is a fresh, + empty store gets the historical corpus instead, and the resulting archive + still looks correct. + """ + from synapt.recall import cli as cli_mod + + cwd_root = tmp_path / "cwd-workspace" + override_root = tmp_path / "override-workspace" + cwd_root.mkdir() + override_root.mkdir() + + _seed_recall_project( + cwd_root, + session_id="sess-cwd", + chunk_id="sess-cwd:t0", + knowledge_id="know-cwd", + journal_focus="cwd focus", + channel_id="msg-cwd", + reminder_id="rem-cwd", + ) + _seed_recall_project( + override_root, + session_id="sess-override", + chunk_id="sess-override:t0", + knowledge_id="know-override", + journal_focus="override focus", + channel_id="msg-override", + reminder_id="rem-override", + ) + + out = tmp_path / "exported.synapt-archive" + monkeypatch.chdir(cwd_root) + monkeypatch.setenv("SYNAPT_RECALL_ROOT", str(override_root)) + + args = argparse.Namespace( + output=str(out), + exclude_transcripts=False, + exclude_channels=False, + ) + cli_mod.cmd_export(args) + + names = "\n".join(_archive_member_names(out)) + + # CONTROL: the two stores must actually differ, or this test passes + # vacuously no matter which root was resolved. + assert "sess-cwd" != "sess-override" + assert "sess-cwd" in _seed_marker(cwd_root), "fixture did not seed the cwd root" + + assert "sess-override" in names, ( + "export resolved the CWD store instead of SYNAPT_RECALL_ROOT.\n" + f"archive members:\n{names}" + ) + assert "sess-cwd" not in names, ( + "export carried the CWD store's content instead of the named store, " + "and the archive would still look correct.\n" + f"archive members:\n{names}" + ) + + +def _seed_marker(project: Path) -> str: + """Return a string proving *project* actually holds seeded content.""" + from synapt.recall.core import project_archive_dir + + return "\n".join(p.name for p in project_archive_dir(project).glob("*.jsonl")) + + +def test_import_resolves_the_recall_root_override_not_the_cwd(tmp_path, monkeypatch): + """`recall import` must land in the store SYNAPT_RECALL_ROOT names. + + Mirror of the export witness. A fresh workspace receiving an archive is + the seeding case: if import resolves the cwd, the archive lands in whatever + store the operator happened to be standing in, not the one they named. + """ + from synapt.recall import cli as cli_mod + from synapt.recall.core import project_archive_dir + + source = tmp_path / "source" + cwd_root = tmp_path / "cwd-workspace" + override_root = tmp_path / "override-workspace" + for d in (source, cwd_root, override_root): + d.mkdir() + + _seed_recall_project( + source, + session_id="sess-seed", + chunk_id="sess-seed:t0", + knowledge_id="know-seed", + journal_focus="seed focus", + channel_id="msg-seed", + reminder_id="rem-seed", + ) + archive, _ = export_recall_archive(source, tmp_path / "seed.synapt-archive") + + monkeypatch.chdir(cwd_root) + monkeypatch.setenv("SYNAPT_RECALL_ROOT", str(override_root)) + args = argparse.Namespace(archive=str(archive), merge=False, replace=True) + cli_mod.cmd_import(args) + + landed_override = list(project_archive_dir(override_root).glob("sess-seed*.jsonl")) + landed_cwd = list(project_archive_dir(cwd_root).glob("sess-seed*.jsonl")) + + assert landed_override, ( + "import did not land in SYNAPT_RECALL_ROOT's store -- the seed went " + "somewhere other than the named store" + ) + assert not landed_cwd, ( + f"import wrote into the CWD store: {[p.name for p in landed_cwd]}" + ) + + +def test_import_reports_the_destination_store_not_the_archives_source(tmp_path, monkeypatch, capsys): + """`recall import` must report the RESOLVED destination store it wrote. + + The archive's manifest carries the SOURCE store's ``data_dir`` (written by + export). A report that echoed that field would name where the bytes came + from and look like a destination. So the witness seeds the archive from + one root, imports under an override root while standing in a third, and + asserts the printed store is the override's data dir and neither the + source's nor the cwd's. Both output paths are driven: the CLI print and + the MCP tool text. + """ + from synapt.recall import cli as cli_mod + from synapt.recall import server as server_mod + from synapt.recall.core import project_data_dir + + source = tmp_path / "source" + cwd_root = tmp_path / "cwd-workspace" + override_root = tmp_path / "override-workspace" + for d in (source, cwd_root, override_root): + d.mkdir() + + _seed_recall_project( + source, + session_id="sess-seed", + chunk_id="sess-seed:t0", + knowledge_id="know-seed", + journal_focus="seed focus", + channel_id="msg-seed", + reminder_id="rem-seed", + ) + archive, manifest = export_recall_archive(source, tmp_path / "seed.synapt-archive") + source_store = str(project_data_dir(source)) + # control: the archive really does carry the source store, so echoing it + # would be a live trap rather than a hypothetical one + assert manifest["data_dir"] == source_store + + monkeypatch.chdir(cwd_root) + monkeypatch.setenv("SYNAPT_RECALL_ROOT", str(override_root)) + dest_store = str(project_data_dir(None)) + assert dest_store == str(project_data_dir(override_root)), "override not live; witness inert" + assert dest_store not in (source_store, str(project_data_dir(cwd_root))) + + # library summary: destination under data_dir, provenance under source_data_dir + summary = import_recall_archive(None, archive, mode="replace") + assert summary["data_dir"] == dest_store, summary + assert summary["source_data_dir"] == source_store, summary + + # CLI print + args = argparse.Namespace(archive=str(archive), merge=False, replace=True) + cli_mod.cmd_import(args) + out = capsys.readouterr().out + assert f"store={dest_store}" in out, out + assert source_store not in out, f"CLI reported the archive's SOURCE store as destination: {out}" + + # MCP tool text + text = server_mod.recall_import(str(archive), mode="replace") + assert f"- store: {dest_store}" in text, text + assert source_store not in text, f"MCP reported the archive's SOURCE store as destination: {text}" + + +def _rewrite_archive_manifest(archive: Path, out: Path, drop_key: str) -> Path: + """Copy *archive* to *out* with *drop_key* removed from manifest.json. + + Builds a pre-provenance archive from a current one, so the legacy shape + is real bytes on disk rather than a patched loader. + """ + import io + import tarfile + + with tarfile.open(archive, "r:*") as src, tarfile.open(out, "w:gz", format=tarfile.PAX_FORMAT) as dst: + for member in src.getmembers(): + payload = src.extractfile(member) if member.isfile() else None + if member.name == "manifest.json" and payload is not None: + manifest = json.loads(payload.read().decode("utf-8")) + manifest.pop(drop_key, None) + data = json.dumps(manifest, indent=2, sort_keys=True).encode("utf-8") + info = tarfile.TarInfo("manifest.json") + info.size = len(data) + dst.addfile(info, io.BytesIO(data)) + else: + dst.addfile(member, payload) + return out + + +@pytest.mark.parametrize("mode", ["replace", "merge"]) +def test_import_omits_source_data_dir_when_the_archive_carries_no_provenance(tmp_path, monkeypatch, mode): + """A pre-provenance archive imports with NO source_data_dir key, in both modes. + + Absent must read as absent: a None here is a value someone later treats + as a path. Control: the same import from the current-format archive DOES + carry source_data_dir, so the absence comes from the manifest, not the + mode. The destination is still reported either way. + """ + from synapt.recall.core import project_data_dir + + source = tmp_path / "source" + dest_root = tmp_path / "dest-workspace" + source.mkdir() + dest_root.mkdir() + _seed_recall_project( + source, + session_id="sess-seed", + chunk_id="sess-seed:t0", + knowledge_id="know-seed", + journal_focus="seed focus", + channel_id="msg-seed", + reminder_id="rem-seed", + ) + current, manifest = export_recall_archive(source, tmp_path / "current.synapt-archive") + assert "data_dir" in manifest # the current format carries provenance + legacy = _rewrite_archive_manifest(current, tmp_path / "legacy.synapt-archive", "data_dir") + assert "data_dir" not in _load_archive_manifest(legacy) # and the legacy one truly does not + + monkeypatch.setenv("SYNAPT_RECALL_ROOT", str(dest_root)) + dest_store = str(project_data_dir(None)) + assert dest_store == str(project_data_dir(dest_root)), "override not live; witness inert" + + # control: current archive -> provenance present + with_prov = import_recall_archive(None, current, mode=mode) + assert with_prov["source_data_dir"] == str(project_data_dir(source)), with_prov + assert with_prov["data_dir"] == dest_store + + # legacy archive -> key absent, destination still reported + without = import_recall_archive(None, legacy, mode=mode) + assert "source_data_dir" not in without, ( + f"{mode}: legacy import carried source_data_dir={without.get('source_data_dir')!r}; absent must read as absent" + ) + assert without["data_dir"] == dest_store, without + + +def test_export_reports_the_data_dir_it_read_not_the_path_it_was_given(tmp_path, monkeypatch): + """The reported store must be the RESOLVED data dir, never the argument. + + ``--path`` hands the resolver an explicit root, but ``project_data_dir`` + still runs git-worktree inference on it. If the manifest records the + argument, an operator who points ``--path`` at a linked worktree sees + ``store=`` while the bytes come from ``
/.synapt/recall``. + A plausible path is the same defect as a plausible count. + + Fixture: a real git repo with a linked worktree, so inference is provably + LIVE -- the control asserts the two candidate dirs differ before the + reporting assertion runs. + """ + import subprocess + + from synapt.recall import cli as cli_mod + from synapt.recall.core import project_data_dir + + main = tmp_path / "main" + main.mkdir() + subprocess.run(["git", "init", "-q", str(main)], check=True) + subprocess.run(["git", "-C", str(main), "commit", "-q", "--allow-empty", "-m", "init"], + check=True, env={"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t", + "PATH": "/usr/bin:/bin:/usr/local/bin"}) + linked = tmp_path / "linked" + subprocess.run(["git", "-C", str(main), "worktree", "add", "-q", str(linked), "-b", "wt"], + check=True) + + _seed_recall_project( + main, + session_id="sess-main", + chunk_id="sess-main:t0", + knowledge_id="know-main", + journal_focus="main focus", + channel_id="msg-main", + reminder_id="rem-main", + ) + + # CONTROL: inference must be live, i.e. the linked worktree must resolve + # to main's data dir. If this fails the test is inert, not passing. + resolved = project_data_dir(linked) + assert resolved == project_data_dir(main), "inference is not live; fixture is inert" + assert resolved != linked / ".synapt" / "recall" + + monkeypatch.delenv("SYNAPT_RECALL_ROOT", raising=False) + monkeypatch.chdir(tmp_path) + out = tmp_path / "wt.synapt-archive" + args = argparse.Namespace(path=str(linked), output=str(out), + exclude_transcripts=False, exclude_channels=False) + cli_mod.cmd_export(args) + + import json, tarfile + with tarfile.open(out, "r:gz") as tf: + manifest = json.load(tf.extractfile("manifest.json")) + + assert manifest["data_dir"] == str(resolved), ( + "manifest named a store it did not read: " + f"reported {manifest['data_dir']} but bytes came from {resolved}" + ) + with tarfile.open(out, "r:gz") as tf: + assert any("sess-main" in n for n in tf.getnames()), "export did not carry main's store" From 939a99cd54067778efc3ee8dd27b87fed9ae353f Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Wed, 19 Aug 2026 04:24:28 -0500 Subject: [PATCH 15/19] fix(journal): stabilize worktree bucket resolution --- src/synapt/recall/core.py | 27 +++++++++- tests/recall/test_gripspace.py | 91 ++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 2 deletions(-) diff --git a/src/synapt/recall/core.py b/src/synapt/recall/core.py index 29b520e0..4f1523ca 100644 --- a/src/synapt/recall/core.py +++ b/src/synapt/recall/core.py @@ -4479,7 +4479,7 @@ def project_slug(project_dir: Path | None = None) -> str: def _worktree_name(project_dir: Path | None = None) -> str: - """Return the current worktree's name (its directory basename). + """Return the stable worktree or workspace name for per-worktree data. For the main worktree at ``/Users/me/Development/rd``, returns ``rd``. For a linked worktree at ``/Users/me/Development/poe``, returns ``poe``. @@ -4501,7 +4501,30 @@ def _worktree_name(project_dir: Path | None = None) -> str: f"per-worktree files outside worktrees/." ) return env_name - return (project_dir or Path.cwd()).resolve().name + + current = (project_dir or Path.cwd()).resolve() + grip_root = _find_gripspace_root(current) + candidate = current + while candidate != candidate.parent: + # A .git marker identifies the root of either a main checkout or a + # linked worktree. It is a directory in the former and a file in the + # latter. Keep walking only until the nearest such root so a + # constituent repository retains its own bucket inside a shared + # gripspace store. + if (candidate / ".git").exists(): + return candidate.name + if (candidate / ".gitgrip" / "griptree.json").exists(): + return candidate.name + if candidate == grip_root: + break + candidate = candidate.parent + + # A non-git directory inside a gripspace still belongs to the workspace. + # This mirrors data-root resolution and prevents a cwd subdirectory from + # becoming a second, silently partial journal bucket. + if grip_root is not None: + return grip_root.name + return current.name # --------------------------------------------------------------------------- diff --git a/tests/recall/test_gripspace.py b/tests/recall/test_gripspace.py index 1ba223c3..1658a1f8 100644 --- a/tests/recall/test_gripspace.py +++ b/tests/recall/test_gripspace.py @@ -408,6 +408,97 @@ def test_all_sub_repos_share_same_data_dir(self, tmp_path): assert result_root == result_a == result_b +def test_worktree_bucket_is_stable_below_a_gr2_workspace(tmp_path): + """A subdirectory must read the workspace's journal, not mint a slice. + + The ``.grip`` marker deliberately holds the data root constant. Before + recall#974's fix, only the bucket changed from ``workspace`` to ``server``. + """ + workspace = _make_gr2_workspace(tmp_path) + nested = workspace / "server" + nested.mkdir() + + root_bucket = project_worktree_dir(workspace) + nested_bucket = project_worktree_dir(nested) + + assert root_bucket == nested_bucket + assert root_bucket == workspace / ".synapt" / "recall" / "worktrees" / "gr2-workspace" + + +def test_gr2_workspace_boundary_beats_an_enclosing_git_root(tmp_path): + """A workspace marker owns its namespace even inside another checkout.""" + (tmp_path / ".git").mkdir() + workspace = _make_gr2_workspace(tmp_path) + nested = workspace / "server" + nested.mkdir() + + assert project_worktree_dir(nested) == ( + workspace / ".synapt" / "recall" / "worktrees" / "gr2-workspace" + ) + + +def test_worktree_bucket_uses_the_repo_root_beneath_a_gripspace(tmp_path): + """Constituent repos share the store but retain distinct stable buckets.""" + grip = _make_gripspace(tmp_path) + repo = _make_git_repo(grip, "repo-a") + nested = repo / "src" / "synapt" + nested.mkdir(parents=True) + + repo_bucket = project_worktree_dir(repo) + nested_bucket = project_worktree_dir(nested) + + assert repo_bucket == nested_bucket + assert repo_bucket == grip / ".synapt" / "recall" / "worktrees" / "repo-a" + + +def test_worktree_bucket_is_stable_below_a_linked_griptree(tmp_path): + """A linked griptree root is its own bucket even without a root .git.""" + grip = _make_gripspace(tmp_path) + main_repo = _make_git_repo(grip, "repo-a") + worktree_dir = main_repo / ".git" / "worktrees" / "dev" + worktree_dir.mkdir(parents=True) + + griptree = tmp_path / "dev-tree" + griptree.mkdir() + (griptree / ".gitgrip").mkdir() + (griptree / ".gitgrip" / "griptree.json").write_text("{}") + linked_repo = griptree / "repo-a" + linked_repo.mkdir() + (linked_repo / ".git").write_text(f"gitdir: {worktree_dir}\n") + nested = griptree / "docs" + nested.mkdir() + + root_bucket = project_worktree_dir(griptree) + nested_bucket = project_worktree_dir(nested) + + assert root_bucket == nested_bucket + assert root_bucket == grip / ".synapt" / "recall" / "worktrees" / "dev-tree" + + +def test_worktree_bucket_uses_a_linked_repo_file_as_its_root(tmp_path): + """A linked repository's .git file is a root marker, not a directory.""" + grip = _make_gripspace(tmp_path) + main_repo = _make_git_repo(grip, "repo-a") + worktree_dir = main_repo / ".git" / "worktrees" / "dev" + worktree_dir.mkdir(parents=True) + + griptree = tmp_path / "dev-tree" + griptree.mkdir() + (griptree / ".gitgrip").mkdir() + (griptree / ".gitgrip" / "griptree.json").write_text("{}") + linked_repo = griptree / "repo-a" + linked_repo.mkdir() + (linked_repo / ".git").write_text(f"gitdir: {worktree_dir}\n") + nested = linked_repo / "src" / "synapt" + nested.mkdir(parents=True) + + root_bucket = project_worktree_dir(linked_repo) + nested_bucket = project_worktree_dir(nested) + + assert root_bucket == nested_bucket + assert root_bucket == grip / ".synapt" / "recall" / "worktrees" / "repo-a" + + class TestProjectTranscriptDirsGripspace: """Tests for gripspace-aware transcript discovery.""" From 309c086cd8af5550537db81e6e359a642872499b Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Wed, 19 Aug 2026 05:01:59 -0500 Subject: [PATCH 16/19] feat(build): incremental by default, a no-op that says so, and `maintain` Three specs in one seam, all of which needed the parser to be testable first. PARSER EXTRACTED. `make_parser()` now builds what `main()` used to build inline. That is not incidental refactoring: while the parser was constructed inside main(), no test could ask what a bare `synapt build` does, which is how the CLI came to default to a FULL rebuild while MCP `recall_build` defaulted to incremental -- the same operation with opposite defaults depending on which surface you reached through, with nothing red anywhere. test_cli_and_mcp_defaults_agree now pins the two together. DEFAULT FLIPPED. Incremental is the default; `--full` is the explicit opt-out and sets BOTH fields rather than leaving one derived from the other, because two fields that must agree are two chances to disagree and this pair has already diverged once. `--incremental` still parses, for scripts that pass it. A NO-OP RUN SAYS SO -- AND SAYS ONLY WHAT IS TRUE, WITNESSED. A fast build and a broken build are indistinguishable from outside unless the build states which stages it skipped, so the short-circuit reports as well as returning early, and returns a usable index because callers dereference it. The line names parse, enrich and index, NOT archive: archiving is Step 1 and has already run by the time the check fires. It copied nothing, which is precisely why the signature matches, but a stage that executed must not be reported as skipped. An operator reads that line and stops looking. The corrected line is WITNESSED. Section F's existing test asserts only that "up to date" and "skip" appear, so it passes on the truthful line and on the false one alike -- verified: restoring the false wording reds ONLY the new test_the_skip_line_does_not_name_a_stage_that_actually_ran while every other skip-related test stays green. A correction nobody can regress is a correction that comes back. THE FINDING, measured rather than reasoned: THE BUILD MUTATES ONE OF ITS OWN SIGNED INPUTS. It synthesizes auto-journal stubs, so the journal on disk after a build differs from the journal it read. Storing the pre-build signature therefore makes the next run compute a different digest from a workspace nobody touched, and the no-op can NEVER fire -- pre 26636950, post f4d4491d, measured on an untouched store. So the signature is recomputed after the build rather than reused from before it. `_build_journal_files` extracted so the signature and the build read the same set. Two independent derivations of "the inputs" are two chances to disagree, and a signature covering fewer files than the build reads yields a FALSE no-op: the build reports "up to date" while real content sits unindexed, the one failure direction with no external symptom. AND THE RECOMPUTE OPENED A SECOND FALSE NO-OP, which took three review rounds to close properly. Recomputing at the end also picks up ARRIVALS: anything landing between the read and the manifest write is stat'd into the stored signature while never having been read, so the next run compares equal, prints "Up to date", and that content stays invisible until some unrelated change moves the digest. Three distinct paths, all measured, all found by review: a transcript dropped after the index save; a journal entry appended after the index save; and an entry landing after the guards had sampled but before a LATER call computed the value actually written to the manifest. The first two fixes were patches on the symptom. Excluding journals from the guard suppressed a false positive and surrendered every true positive with it, and adding a per-class journal check left the compared samples and the persisted sample as different objects -- so the guard verified one value and stored another, which verifies nothing about what was stored. WHAT IT NEEDED WAS ONE SAMPLE, TAKEN EARLY, VALIDATED LATE. A single full signature is computed at the READ BOUNDARY -- stub synthesis complete, journal parsing not yet begun, the exact line where the build's own writes end and somebody else's begin -- and THAT OBJECT is what gets persisted. Two comparisons decide only whether to keep it: read-only inputs must not have moved between the start of the run and the read boundary (journals excluded from this one alone, because the build writes them in that window), and the full input set must be unchanged between the read boundary and the end. Because the stored value is older than both comparisons, nothing arriving afterwards -- including after the check itself -- can be inside it. The distinction that matters is a property of TIME, before-the-read versus after-the-read, not of input class. SIGN WHAT THE BUILD PARSES, NOT WHAT IT COPIES FROM. Step 1 copies transcripts into the project archive and every later stage parses THAT archive. The signature was keyed on source_dirs -- the outside directories copied FROM -- and that is a different set. The gap is not theoretical: Codex rollouts are copied straight from the Codex sessions directory and never pass through source_dirs at all, so a Codex workspace printed "Archived 1 Codex transcript(s)" and "Up to date" in the same run and never parsed it. The same mis-keying mis-times ordinary transcripts: one written after Step 1's copy is present in source_dirs and absent from the archive, so it is signed as parsed without having been, and the next run copies it and then skips. All four signature computations are now keyed on build_sources. Both paths witnessed, both red on the prior revision. (Stromus, r1 on v6, offered as one shape closing both; taken as offered.) AN INPUT THE BUILD READS MUST BE AN INPUT THE SIGNATURE COVERS. The ChatGPT export is parsed into the index on every run and was absent from the signature, so replacing the archive wholesale left the digest unchanged and the next run skipped it. That is the narrower sibling of the coverage gap this same seam fixes for journals -- the same class, in the same patch, one input over. `maintain` is the grinder's explicit home, bounded by default, and REPORTS the backlog rather than draining it silently -- a queue being worked and a queue stuck look identical unless the number left is printed. It calls the grinder as a module attribute, not a from-imported name, so a runtime swap is not silently ignored. Evidence: 9 red specs green. The suite is GREEN -- 2929 passed, 25 skipped, 5 xfailed, 0 failed -- and the invocation is stated because the count is not reproducible without it: from synapt/, `.venv/bin/python -m pytest`, with the import resolving to this worktree's src/synapt/recall/__init__.py, printed before the run. An earlier revision of this message claimed 2978 passing with 5 environmental failures. That measurement was taken under a pyenv interpreter whose `synapt.recall` resolved to a DIFFERENT worktree entirely, so it described code this branch does not contain; the failures were that interpreter's, not this suite's. The correction is recorded rather than quietly replaced, because a count that moves without an explanation is indistinguishable from a count that was never checked. Mutation table, re-derived against the final shape rather than carried forward. Persisting the pre-build signature reds the two skip-line witnesses. Forcing the no-op unconditionally reds 28. Dropping archive coverage reds the archive witness alone. Dropping the read-boundary-to-end comparison, and persisting a fresh sample instead of the compared one, each red the persisted-signature witness -- they share it by construction, being two ways to break one property. Re-keying all four signature sites back to source_dirs reds the Codex and post-copy witnesses; re-keying only one of them reds the skip-line witnesses instead, because the pre-build and persisted samples must agree. EVERY GUARD IS WITNESSED, AND THE ONE THAT NEARLY WAS NOT IS WORTH NAMING. The start-to-read-boundary comparison originally guarded mid-parse transcript arrivals; keying on build_sources subsumed that case and the mutation stopped reddening anything, leaving it responsible for channels and the archive, which are parsed before the journal read boundary. My first witness for it resolved the channels directory by INFERENCE and was defeated by its own inference, which I misreported as test isolation preventing the witness. It does not: pinning store resolution through the explicit env seam lands it, and removing the guard reds that witness and nothing else. Both arrival witnesses survive every single mutation and go red only when the end comparison and the persist-the-compared-object property are removed TOGETHER: two independent mechanisms each covering that case, measured rather than asserted. All restores verified against a clean baseline. Co-Authored-By: Claude --- src/synapt/recall/build_delta.py | 19 +- src/synapt/recall/cli.py | 329 ++++++++++++++- tests/recall/test_build_idempotence.py | 535 +++++++++++++++++++++++++ 3 files changed, 858 insertions(+), 25 deletions(-) diff --git a/src/synapt/recall/build_delta.py b/src/synapt/recall/build_delta.py index 059a4f22..b8ec4780 100644 --- a/src/synapt/recall/build_delta.py +++ b/src/synapt/recall/build_delta.py @@ -98,8 +98,15 @@ def _iter_input_files( source_dirs: Sequence[Path] | None, channels_dir: Path | None, journal_paths: Iterable[Path] | None, + archive_paths: Iterable[Path] | None = None, ) -> list[Path]: - """Every file the build would read, from all three input classes. + """Every file the build would read, from all FOUR input classes. + + `archive_paths` covers the ChatGPT export, which the build parses into the + index on every run and which this function did not see: replacing the + archive wholesale left the digest unchanged and the next run skipped it. + An input the build READS and the signature does not COVER makes the no-op + a decision over a strict subset of the truth (Atlas, r2 on v3). Directories are GLOBBED rather than read from a remembered list, so a newly arrived file changes the signature. A signature built only from files it @@ -123,6 +130,11 @@ def _iter_input_files( if journal.is_file(): files.append(journal) + for archive in archive_paths or (): + archive = Path(archive) + if archive.is_file(): + files.append(archive) + return files @@ -130,8 +142,9 @@ def compute_input_signature( source_dirs: Sequence[Path] | None = None, channels_dir: Path | None = None, journal_paths: Iterable[Path] | None = None, + archive_paths: Iterable[Path] | None = None, ) -> InputSignature: - """Fingerprint the build's inputs across transcripts, channels and journals. + """Fingerprint the build's inputs: transcripts, channels, journals, archives. Sorted before hashing so the digest depends on the input set and not on filesystem iteration order, which varies between runs and would otherwise @@ -140,7 +153,7 @@ def compute_input_signature( does so *intermittently* is worse, because it trains people to distrust it. """ entries: list[str] = [] - for path in _iter_input_files(source_dirs, channels_dir, journal_paths): + for path in _iter_input_files(source_dirs, channels_dir, journal_paths, archive_paths): try: stat = path.stat() except OSError: diff --git a/src/synapt/recall/cli.py b/src/synapt/recall/cli.py index 5e9933dc..522e2523 100644 --- a/src/synapt/recall/cli.py +++ b/src/synapt/recall/cli.py @@ -204,6 +204,28 @@ def _release_build_lock(fd: int) -> None: os.close(fd) +def _build_journal_files(project_dir: Path) -> list[Path]: + """Every journal file this build reads: local, plus each other worktree's. + + Extracted so the no-op SIGNATURE and the build itself cannot read different + sets. Two independent derivations of "the inputs" are two chances to + disagree, and a signature covering fewer files than the build reads produces + a FALSE no-op -- the build reports "up to date" while a real change sits + unindexed, which is the one failure direction with no external symptom. + """ + from synapt.recall.journal import _journal_path + + local_journal = _journal_path(project_dir) + files = [local_journal] + for wt_archive in all_worktree_archive_dirs(project_dir): + # Archive dir is
/.synapt/recall/worktrees//transcripts/; + # the journal sits beside it at ...//journal.jsonl + wt_journal = wt_archive.parent / "journal.jsonl" + if wt_journal.resolve() != local_journal.resolve() and wt_journal.exists(): + files.append(wt_journal) + return files + + def _archive_and_build( project_dir: Path, source_dirs: list[Path] | None = None, @@ -291,6 +313,73 @@ def _archive_and_build_locked( else: db = RecallDB(index_dir / "recall.db") + # Fingerprint the inputs and short-circuit a no-op run. + # + # NOT before all work: archiving (Step 1) has already run and scanned the + # source dirs by this point. It copied nothing -- which is exactly why the + # signature matches, since the signature is path, size and mtime -- but the + # skip line must not claim a stage was skipped when it executed. An operator + # reads that line and stops looking, so it says what actually happened. + # + # A fast build and a broken build look identical from outside unless the + # build says which stages it skipped, so the no-op path REPORTS as well as + # returning early. Computed unconditionally, not only when incremental, so a + # full rebuild still leaves a baseline for the next incremental run -- + # otherwise the first incremental build after any full one can never be a + # no-op and the signal looks broken. + # + # `is_noop` fails toward doing the work when the prior signature is absent, + # malformed, or version-mismatched. That asymmetry is deliberate: a wrong + # "changed" costs one unnecessary build, while a wrong "unchanged" leaves + # real content unindexed and prints a reassuring line about it. + from synapt.recall.build_delta import ( + compute_input_signature, + is_noop, + signature_from_manifest, + signature_to_manifest, + ) + from synapt.recall.channel import _channels_dir + + # The ChatGPT export is parsed into the index on every build, so it is a + # build input and must be a signed one (Atlas, r2 on v3). + archive_paths = [Path(chatgpt_archive).expanduser()] if chatgpt_archive else [] + + def _readonly_signature(): + """Signature over the inputs the build only READS. + + Keyed on `build_sources` -- the ARCHIVE the build actually parses -- and + not on `source_dirs`, the outside directories Step 1 copies FROM. Signing + the copy source cannot see anything reaching the archive by another route + (Codex rollouts are copied straight from the Codex sessions directory and + never pass through source_dirs at all), and it mis-times everything else: + a transcript written after Step 1's copy is present in source_dirs and + absent from the archive, so it gets signed as parsed without having been. + Sign what is PARSED, not what it was copied from (Stromus, r1 on v6). + + Journals are excluded deliberately: the build WRITES them (auto-journal + stubs), so they differ before and after by design and cannot be used to + detect an outside arrival. Everything here is content the build treats + as immutable for the duration of the run, which makes a difference + between two samples of it proof that something landed mid-build. + """ + return compute_input_signature( + source_dirs=build_sources, + channels_dir=_channels_dir(project_dir), + archive_paths=archive_paths, + ) + + readonly_before = _readonly_signature() + build_signature = compute_input_signature( + source_dirs=build_sources, + channels_dir=_channels_dir(project_dir), + journal_paths=_build_journal_files(project_dir), + archive_paths=archive_paths, + ) + if incremental and is_noop(signature_from_manifest(db.load_manifest()), build_signature): + print(" Up to date: no transcript, channel or journal input changed") + print(" Skipped: parse, enrich, index (archive scanned; nothing new)") + return TranscriptIndex.load(index_dir) + # Step 4: Load existing data for incremental builds incremental_manifest = None existing_chunks = [] @@ -392,17 +481,11 @@ def _archive_and_build_locked( from synapt.recall.journal import _journal_path, synthesize_journal_stubs from synapt.recall.core import parse_journal_entries - # Collect journal files from all worktrees for the shared index - journal_files: list[Path] = [] + journal_files = _build_journal_files(project_dir) + # Recomputed rather than taken as journal_files[0]: relying on the + # helper's ordering would make a harmless reordering there a silent + # bug here. local_journal = _journal_path(project_dir) - journal_files.append(local_journal) - # Also include journals from other worktrees - for wt_archive in all_worktree_archive_dirs(project_dir): - # Archive dir is
/.synapt/recall/worktrees//transcripts/ - # Journal is at
/.synapt/recall/worktrees//journal.jsonl - wt_journal = wt_archive.parent / "journal.jsonl" - if wt_journal.resolve() != local_journal.resolve() and wt_journal.exists(): - journal_files.append(wt_journal) transcript_chunks = [c for c in all_chunks if c.turn_index >= 0] if transcript_chunks: @@ -414,6 +497,33 @@ def _archive_and_build_locked( if synthesized: print(f" Auto-journal: {synthesized} stub(s) synthesized") + # THE JOURNAL READ BOUNDARY. Stub synthesis is done; parsing has not begun. + # This is the journal state the index is about to reflect, and it is the + # only sample that can tell the build's OWN writes from an outside arrival: + # everything before this line is ours, everything after it is somebody + # else's. v4 excluded journals from the arrival guard entirely to avoid + # mistaking synthesis for an arrival -- correct about our writes, and it + # surrendered every genuine journal arrival along with them (Atlas, r2 on + # v4: an entry appended after the index save was certified as indexed and + # the next run returned zero of it). Excluding a class to suppress a false + # positive gives up the true positives in the same motion. + # ONE FULL SAMPLE, TAKEN HERE, AND IT IS THE ONE THAT GETS PERSISTED. + # v5 compared journals, then compared read-only inputs, then computed a + # THIRD signature to store -- so the value written to the manifest was never + # the value that was checked, and anything arriving in between was certified + # as indexed without being read (Atlas, r2 on v5: measured, three signature + # computations after the index write). Two samples of the same quantity are + # not the same sample. The stored signature is therefore taken HERE, before + # any guard runs, and the guards only decide whether to keep it: nothing that + # happens later can enter a value that was already computed. + signature_at_read = compute_input_signature( + source_dirs=build_sources, + channels_dir=_channels_dir(project_dir), + journal_paths=journal_files, + archive_paths=archive_paths, + ) + readonly_at_read = _readonly_signature() + # Journal entries → searchable chunks from ALL worktrees for journal_file in journal_files: if journal_file.exists(): @@ -651,7 +761,69 @@ def _archive_and_build_locked( "mtime": st.st_mtime, "size": st.st_size, }) - db.save_manifest({"source_files": source_files}) + # Persist the signature alongside the file list so the NEXT run can tell + # "nothing changed" from "no idea". Written in the same call, because a + # manifest that has the file list but not the signature is a state where + # the no-op check silently never fires. + # RECOMPUTED, not the pre-build value. The build MUTATES ONE OF ITS OWN + # SIGNED INPUTS: it synthesizes auto-journal stubs, so the journal on disk + # after a build differs from the journal it read. Storing the pre-build + # signature means the next run computes a different digest from an unchanged + # workspace and the no-op can NEVER fire -- measured: pre 26636950, + # post f4d4491d, on a workspace nobody touched in between. + # + # What is stored is therefore "the input state as of the end of this build", + # which is exactly what the next run's pre-build signature is compared against. + # + # BUT A RECOMPUTE ALSO PICKS UP ARRIVALS. Anything that landed between the + # index write and this line gets stat'd into the stored signature while + # never having been read, so the next run compares equal, prints "Up to + # date", and the content is invisible until something else happens to + # change the digest. Measured on v3: a transcript dropped after the index + # save was certified as indexed and returned zero content (Atlas, r2). + # + # So the signature is stored ONLY when the read-only inputs are unchanged + # since the build read them. When they are not, the manifest keeps its file + # list and carries NO signature, which `signature_from_manifest` resolves to + # None and `is_noop` resolves to work. Failing toward one unnecessary build + # is the same asymmetry the no-op check already documents, applied to the + # one window where the build cannot vouch for its own inputs. + # + # TWO WINDOWS, TWO COMPARISONS, AND THE PERSISTED VALUE IS OLDER THAN BOTH. + # 1. start -> read boundary: read-only inputs must not have moved while + # transcripts, channels and the archive were being parsed. Journals are + # excluded from THIS one only, because the build writes them in this + # window and would otherwise flag its own synthesis as an arrival. + # 2. read boundary -> now: the full input set, all four classes, must be + # unchanged. Recomputed over the CURRENT sets rather than the ones read, + # so a file that APPEARED mid-build also fails -- the safe direction. + # What is stored is `signature_at_read`, sampled before either comparison, + # so an arrival at any later point -- including after this very check -- + # cannot be inside it. The next run then computes a different digest and + # does the work. + # + # The first comparison is witnessed by the channel-arrival test. It + # originally guarded mid-parse TRANSCRIPT arrivals; keying the signature on + # `build_sources` subsumed that case, leaving it responsible for channels + # and the archive, which are parsed before the journal read boundary. + # Its witness pins store resolution through the explicit env seam rather + # than inferring it from cwd -- an earlier attempt inferred, was defeated by + # its own inference, and was wrongly reported as blocked by test isolation. + manifest_payload = {"source_files": source_files} + inputs_stable = ( + readonly_at_read.digest == readonly_before.digest + and compute_input_signature( + source_dirs=build_sources, + channels_dir=_channels_dir(project_dir), + journal_paths=_build_journal_files(project_dir), + archive_paths=archive_paths, + ).digest == signature_at_read.digest + ) + if inputs_stable: + manifest_payload.update(signature_to_manifest(signature_at_read)) + else: + print(" Note: input changed during the build; next run will not skip") + db.save_manifest(manifest_payload) logger.info("build: complete in %.1fs (%d chunks)", _time.monotonic() - build_t0, len(deduped)) return final_index @@ -2793,17 +2965,84 @@ def cmd_migrate_channels(args: "argparse.Namespace") -> None: # Argparse # --------------------------------------------------------------------------- -def main(): - # Configure logging so build progress is visible on stderr. - # Only set up if no handlers exist yet (avoid duplicate output when - # called from the MCP server, which configures its own logging). - if not logging.getLogger("synapt").handlers: - logging.basicConfig( - level=logging.INFO, - format="%(message)s", - stream=sys.stderr, +def cmd_maintain(args: argparse.Namespace) -> None: + """Grind LLM cluster summaries on request, bounded, and report the backlog. + + This is the grinder's explicit home. `build` no longer calls it, because an + unbounded grind inside a build made a routine rebuild unpredictably slow and + gave the operator no way to decline it. + + The backlog is REPORTED, never silently drained: a queue that is being + worked and a queue that is stuck look identical from outside unless the + number left is printed, so it is printed even when it is zero. + """ + # Import the MODULE, not the name. The grinder is swapped at runtime -- by + # tests, and by anyone pointing it at a different backend -- and a + # `from ... import upgrade_large_cluster_summaries` binds at import time, so + # the swap would be silently ignored while everything still looked correct. + from synapt.recall import clustering + from synapt.recall.core import project_data_dir + from synapt.recall.storage import RecallDB + + min_chunks = 5 + index_dir = project_data_dir(None) / "index" + if not index_dir.exists(): + print(f"No recall index at {index_dir}; run `synapt build` first.") + return + + from synapt.recall.sharding import is_sharded + if is_sharded(index_dir): + from synapt.recall.sharded_db import ShardedRecallDB + db = ShardedRecallDB.open(index_dir) + else: + db = RecallDB(index_dir / "recall.db") + + try: + upgraded = clustering.upgrade_large_cluster_summaries( + db, min_chunks=min_chunks, max_upgrades=args.limit ) + # Count what is LEFT with the grinder's own eligibility criteria rather + # than a paraphrase of them: a backlog measured by a slightly different + # query is a number about a different question. + remaining = db._conn.execute( + "SELECT COUNT(*) " + "FROM clusters c " + "LEFT JOIN cluster_summaries cs " + " ON c.cluster_id = cs.cluster_id AND cs.method = 'llm' " + "WHERE c.status = 'active' " + " AND c.chunk_count >= ? " + " AND cs.cluster_id IS NULL", + (min_chunks,), + ).fetchone()[0] + finally: + db.close() + + print(f"maintain: upgraded {upgraded} cluster summar{'y' if upgraded == 1 else 'ies'}") + print(f" {remaining} remaining above the {min_chunks}-chunk threshold") + + +class _FullRebuild(argparse.Action): + """``--full`` sets BOTH ``full`` and ``incremental`` rather than leaving one + to be derived from the other. + + A reader of the parsed namespace should not have to compute + ``incremental = not full``: two fields that must agree are two chances to + disagree, and this pair has already diverged once across two surfaces. + """ + + def __call__(self, parser, namespace, values, option_string=None): + namespace.full = True + namespace.incremental = False + + +def make_parser() -> argparse.ArgumentParser: + """Build the full `synapt` argument parser. + Extracted from ``main()`` so DEFAULTS ARE TESTABLE. While the parser was + built inline, no test could ask what a bare ``synapt build`` actually + does, which is how the CLI and the MCP surface drifted to opposite + defaults for the same operation without anything going red. + """ parser = argparse.ArgumentParser( prog="synapt", description="Persistent conversational memory for Claude Code sessions (per-project)", @@ -2824,7 +3063,25 @@ def main(): build_parser.add_argument("--chatgpt-archive", help="Path to ChatGPT export .zip (or conversations.json)") build_parser.add_argument("--out", default=None, help="Output directory for index (default: per-project)") build_parser.add_argument("--no-embeddings", action="store_true", help="Skip embeddings (BM25-only, faster build)") - build_parser.add_argument("--incremental", action="store_true", help="Skip already-indexed files") + # DEFAULT: incremental. `--full` is the explicit opt-out. + # + # The CLI defaulted to a FULL rebuild while MCP `recall_build` defaulted to + # incremental: the same operation with opposite defaults depending on which + # surface you reached through. Nothing went red because the parser was built + # inline inside main(), so no test could ask what a bare `synapt build` does. + # test_cli_and_mcp_defaults_agree now pins the two together; changing either + # default alone fails it. + build_parser.set_defaults(incremental=True, full=False) + _build_mode = build_parser.add_mutually_exclusive_group() + _build_mode.add_argument( + "--full", action=_FullRebuild, nargs=0, + help="Rebuild everything from scratch (opt out of the incremental default)", + ) + _build_mode.add_argument( + "--incremental", action="store_true", + help="Skip already-indexed files. Now the default; still accepted so " + "scripts and hooks that pass it explicitly keep working.", + ) build_parser.add_argument("--rescrub", action="store_true", help="Re-scrub archived transcripts with latest patterns before building") # Split @@ -3017,6 +3274,17 @@ def main(): channel_parser.add_argument("--name", default=None, help="Display name for join action") + maintain_parser = subparsers.add_parser( + "maintain", + help="Upgrade cluster summaries with an LLM, bounded, and report the backlog", + ) + maintain_parser.add_argument( + "--limit", type=int, default=5, + help="Maximum summaries to generate this run (default: 5). Bounded by " + "default on purpose: an unbounded grind is what this command exists " + "to replace.", + ) + migrate_parser = subparsers.add_parser( "migrate", help="Migrate local .synapt/recall/channels/ to global ~/.synapt/channels/ store", @@ -3033,10 +3301,27 @@ def main(): "--project", default=None, help="Project ID (auto-detected from gripspace manifest if not set)", ) + return parser + + +def main(): + # Configure logging so build progress is visible on stderr. + # Only set up if no handlers exist yet (avoid duplicate output when + # called from the MCP server, which configures its own logging). + if not logging.getLogger("synapt").handlers: + logging.basicConfig( + level=logging.INFO, + format="%(message)s", + stream=sys.stderr, + ) + + parser = make_parser() args = parser.parse_args() - if args.command == "setup": + if args.command == "maintain": + cmd_maintain(args) + elif args.command == "setup": cmd_setup(args) elif args.command == "build": cmd_build(args) diff --git a/tests/recall/test_build_idempotence.py b/tests/recall/test_build_idempotence.py index 6bf5b177..7a689b09 100644 --- a/tests/recall/test_build_idempotence.py +++ b/tests/recall/test_build_idempotence.py @@ -630,6 +630,43 @@ def test_second_build_reports_that_it_skipped(tmp_path, capsys): assert "skip" in out, f"a no-op build did not report skipped stages:\n{out}" +def test_the_skip_line_does_not_name_a_stage_that_actually_ran(tmp_path, capsys): + """The skip line is load-bearing prose: an operator reads it and stops looking. + + `archive_transcripts` is Step 1 and has ALREADY RUN by the time the no-op + check fires. It copied nothing -- which is exactly why the signature matches + -- but a stage that executed must not be listed as skipped. + + The test above asserts only that "up to date" and "skip" appear, so it + passes on a truthful line and on a false one alike. This pins which stages + the line is allowed to name, in both directions: the three that really were + skipped must be there, and archive must not be among them. + """ + from synapt.recall.cli import _archive_and_build + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + capsys.readouterr() + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + out = capsys.readouterr().out + + skip_lines = [ln for ln in out.splitlines() if "skipped" in ln.lower()] + assert skip_lines, f"no skip line to check:\n{out}" + # Only the claim itself, not any parenthetical explaining what DID run. + claimed = skip_lines[0].lower().split("(")[0] + assert "archive" not in claimed, ( + f"the skip line claims archive was skipped, but archiving is Step 1 and " + f"ran before the check:\n{skip_lines[0]}" + ) + for stage in ("parse", "enrich", "index"): + assert stage in claimed, f"skip line does not name {stage}:\n{skip_lines[0]}" + + def test_second_build_after_a_change_does_not_claim_to_be_up_to_date(tmp_path, capsys): """NEGATIVE CONTROL for the message itself. @@ -720,3 +757,501 @@ def test_cli_and_mcp_defaults_agree(): assert cli_default == mcp_default, ( f"CLI default ({cli_default}) and MCP default ({mcp_default}) disagree" ) + + +# =========================================================================== +# F. The stored signature must describe what the index actually CONTAINS +# =========================================================================== +# +# The signature is recomputed AFTER the build, because the build mutates one of +# its own signed inputs (it synthesizes auto-journal stubs), so a pre-build +# signature could never match on an untouched workspace. That fix opened a +# window: anything arriving between the index save and the recompute is stat'd +# into the stored signature while never reaching the index. The next run then +# compares equal, prints "Up to date", and the content is invisible forever -- +# a false no-op, which is the one direction this whole feature must not fail in. +# +# Both witnesses below fail on the v3 implementation. (Atlas, r2 on v3.) + +def _chatgpt_conversation(conv_id: str, text: str) -> dict: + """A minimal but genuinely parseable ChatGPT export conversation.""" + from conftest import chatgpt_message + return { + "id": conv_id, + "title": conv_id, + "create_time": 1769000000.0, + "update_time": 1769000600.0, + "current_node": "n2", + "mapping": { + "n0": {"id": "n0", "parent": None, "children": ["n1"], "message": None}, + "n1": {"id": "n1", "parent": "n0", "children": ["n2"], + "message": chatgpt_message("user", f"question about {text}")}, + "n2": {"id": "n2", "parent": "n1", "children": [], + "message": chatgpt_message("assistant", f"answer about {text}")}, + }, + } + + +def _chunk_text(chunk) -> str: + return f"{chunk.user_text}\n{chunk.assistant_text}" + + +def test_a_transcript_arriving_during_the_build_is_not_certified_as_indexed( + tmp_path, capsys, monkeypatch +): + """Content that arrived after the index was written must not be signed for. + + The stored signature is the next run's whole basis for skipping work. If it + describes disk at manifest time rather than the input state the index was + built from, a file that landed in between is certified as indexed without + ever having been read -- and the run that would have caught it is exactly + the run that skips. + """ + from synapt.recall.cli import _archive_and_build + from synapt.recall.core import TranscriptIndex + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + real_save = TranscriptIndex.save + arrived = {"yet": False} + + def save_then_a_transcript_arrives(self, directory): + result = real_save(self, directory) + if not arrived["yet"]: + arrived["yet"] = True + _transcript(source / "late.jsonl", turns=6, prefix="LATEARRIVALSENTINEL") + return result + + monkeypatch.setattr(TranscriptIndex, "save", save_then_a_transcript_arrives) + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + monkeypatch.undo() + assert arrived["yet"], "the arrival never fired; this witness proved nothing" + capsys.readouterr() + + index = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + out = capsys.readouterr().out + + hits = [c for c in index.chunks if "LATEARRIVALSENTINEL" in _chunk_text(c)] + assert hits, ( + "a transcript that arrived after the index was saved was folded into the " + "stored signature, so the next run reported nothing to do and the content " + f"was never indexed:\n{out}" + ) + + +def test_a_changed_chatgpt_archive_defeats_the_noop(tmp_path, capsys): + """Every input the build READS must be an input the signature COVERS. + + `chatgpt_archive` is parsed into the index on every build and was absent + from the signature, so replacing the archive wholesale left the digest + unchanged and the next run skipped. The sibling of the coverage gap this + file already pins for journals: the signature and the build must read the + same set, or the no-op is computed over a strict subset of the truth. + """ + from synapt.recall.cli import _archive_and_build + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + archive = tmp_path / "conversations.json" + archive.write_text(json.dumps([_chatgpt_conversation("c1", "the first topic")])) + + _archive_and_build( + project, source_dirs=[source], use_embeddings=False, incremental=True, + chatgpt_archive=str(archive), + ) + capsys.readouterr() + + archive.write_text(json.dumps([ + _chatgpt_conversation("c1", "the first topic"), + _chatgpt_conversation("c2", "CHATGPTLATESENTINEL"), + ])) + _set_mtime(archive, time.time() + 10) + + index = _archive_and_build( + project, source_dirs=[source], use_embeddings=False, incremental=True, + chatgpt_archive=str(archive), + ) + out = capsys.readouterr().out + + assert "up to date" not in out.lower(), ( + f"the archive changed and the build called itself up to date:\n{out}" + ) + hits = [c for c in index.chunks if "CHATGPTLATESENTINEL" in _chunk_text(c)] + assert hits, f"a changed ChatGPT archive was never re-read:\n{out}" + + +def test_a_journal_entry_arriving_during_the_build_is_not_certified_as_indexed( + tmp_path, capsys, monkeypatch +): + """The arrival guard must cover journals too, not exclude the class. + + v4 excluded journals from the read-only comparison because the build WRITES + them (auto-journal stubs), so they differ by design. Correct about the + build's own writes, and it made every GENUINE journal arrival invisible: + excluding a class to avoid a false positive surrenders the true positives + with it. The distinction the guard needs is not journal-vs-other, it is + before-the-read vs after-the-read. (Atlas, r2 on v4.) + """ + from synapt.recall.cli import _archive_and_build + from synapt.recall.core import TranscriptIndex + from synapt.recall.journal import JournalEntry, append_entry, _journal_path + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + real_save = TranscriptIndex.save + arrived = {"yet": False} + + def save_then_a_journal_entry_arrives(self, directory): + result = real_save(self, directory) + if not arrived["yet"]: + arrived["yet"] = True + append_entry( + JournalEntry( + timestamp="2026-08-19T12:00:00+00:00", + session_id="journal-late-arrival", + focus="JOURNALLATESENTINEL arrived after the index was written", + auto=False, + ), + _journal_path(project), + ) + return result + + monkeypatch.setattr(TranscriptIndex, "save", save_then_a_journal_entry_arrives) + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + monkeypatch.undo() + assert arrived["yet"], "the arrival never fired; this witness proved nothing" + capsys.readouterr() + + index = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + out = capsys.readouterr().out + + hits = [c for c in index.chunks if "JOURNALLATESENTINEL" in _chunk_text(c)] + assert hits, ( + "a journal entry that arrived after the index was saved was folded into " + "the stored signature, so the next run reported nothing to do and the " + f"entry was never indexed:\n{out}" + ) + + +def test_the_persisted_signature_is_the_one_that_was_compared( + tmp_path, capsys, monkeypatch +): + """The guard must certify the SAME object it validated. + + v5 compared journals, then compared read-only inputs, then computed a THIRD + full signature to persist. Anything arriving after the comparisons and + before that third call is absent from the index, present in the stored + signature, and certified as indexed on the next run. Two samples of the same + quantity are not the same sample, and a guard that checks one while + persisting the other has verified nothing about what it wrote. + + The trigger fires on the first signature computation AFTER the index has + been written, which exists in both the defective and the corrected shape -- + so this cannot pass by never having fired. (Atlas, r2 on v5.) + """ + from synapt.recall import build_delta + from synapt.recall.cli import _archive_and_build + from synapt.recall.core import TranscriptIndex + from synapt.recall.journal import JournalEntry, append_entry, _journal_path + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + state = {"index_saved": False, "arrival_fired": False, "calls_after_save": 0} + real_save = TranscriptIndex.save + real_signature = build_delta.compute_input_signature + + def save_marking_the_boundary(self, directory): + result = real_save(self, directory) + state["index_saved"] = True + return result + + def signature_then_a_late_arrival(*args, **kwargs): + result = real_signature(*args, **kwargs) + if state["index_saved"]: + state["calls_after_save"] += 1 + if not state["arrival_fired"]: + state["arrival_fired"] = True + append_entry( + JournalEntry( + timestamp="2026-08-19T13:00:00+00:00", + session_id="post-guard-arrival", + focus="POSTGUARDSENTINEL landed after the guard sampled", + auto=False, + ), + _journal_path(project), + ) + return result + + monkeypatch.setattr(TranscriptIndex, "save", save_marking_the_boundary) + monkeypatch.setattr(build_delta, "compute_input_signature", signature_then_a_late_arrival) + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + monkeypatch.undo() + + assert state["index_saved"], "the index was never saved; this witness proved nothing" + assert state["arrival_fired"], ( + "no signature was computed after the index was written, so the arrival " + "never fired and this witness proved nothing" + ) + capsys.readouterr() + + index = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + out = capsys.readouterr().out + + hits = [c for c in index.chunks if "POSTGUARDSENTINEL" in _chunk_text(c)] + assert hits, ( + "an entry arriving after the guard sampled was written into the stored " + "signature by a later, uncompared computation, so the next run reported " + f"nothing to do and never indexed it " + f"({state['calls_after_save']} signature calls after the index save):\n{out}" + ) + + +def test_a_transcript_arriving_before_the_read_boundary_is_not_certified( + tmp_path, capsys, monkeypatch +): + """The window between build start and the journal read boundary is guarded. + + Found by mutation, not by review: disabling the start-to-read-boundary + comparison killed no test, which means that guard was load-bearing and + unwitnessed. An arrival while transcripts are being parsed is absent from + the index but PRESENT in the read-boundary sample, so it would be persisted + as certified and the next run would skip. + + A guard whose removal breaks nothing is indistinguishable from a guard that + does nothing, and the difference only shows up in production. + """ + from synapt.recall import core as recall_core + from synapt.recall.cli import _archive_and_build + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + state = {"fired": False} + real_build_index = recall_core.build_index + + def build_index_then_a_transcript_arrives(*args, **kwargs): + result = real_build_index(*args, **kwargs) + if not state["fired"]: + state["fired"] = True + _transcript(source / "midparse.jsonl", turns=6, prefix="MIDPARSESENTINEL") + return result + + monkeypatch.setattr(recall_core, "build_index", build_index_then_a_transcript_arrives) + monkeypatch.setattr("synapt.recall.cli.build_index", build_index_then_a_transcript_arrives) + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + monkeypatch.undo() + assert state["fired"], "the arrival never fired; this witness proved nothing" + capsys.readouterr() + + index = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + out = capsys.readouterr().out + + hits = [c for c in index.chunks if "MIDPARSESENTINEL" in _chunk_text(c)] + assert hits, ( + "a transcript that arrived while transcripts were being parsed was " + "sampled into the read-boundary signature it never reached the index " + f"through, so the next run skipped:\n{out}" + ) + + +# --------------------------------------------------------------------------- +# G. Sign what the build PARSES, not what it copies FROM +# --------------------------------------------------------------------------- +# +# Step 1 copies transcripts into the project archive; every later stage parses +# the ARCHIVE (`build_sources`). The signature signed `source_dirs` -- the +# outside directories it copies from -- so anything that reaches the archive by +# another route, or after the sample, is parsed without ever being signed. +# Third instance of the same class this file already pins for journals and for +# the ChatGPT export. (Stromus, r1 on v6.) + +def _codex_rollout(sessions_dir: Path, project_dir: Path, name: str, text: str) -> Path: + sessions_dir.mkdir(parents=True, exist_ok=True) + entries = [ + {"timestamp": "2026-03-01T10:00:00Z", "type": "session_meta", + "payload": {"id": name, "cwd": str(project_dir)}}, + {"timestamp": "2026-03-01T10:00:01Z", "type": "response_item", + "payload": {"role": "user", "content": [{"type": "input_text", "text": f"question {text}"}]}}, + {"timestamp": "2026-03-01T10:00:02Z", "type": "response_item", + "payload": {"role": "assistant", "content": [{"type": "output_text", "text": f"answer {text}"}]}}, + ] + path = sessions_dir / f"rollout-{name}.jsonl" + with open(path, "w", encoding="utf-8") as f: + for e in entries: + f.write(json.dumps(e) + "\n") + return path + + +def test_a_new_codex_transcript_defeats_the_noop(tmp_path, capsys, monkeypatch): + """Codex transcripts reach the archive the build parses, by their own route. + + They are copied in Step 1 from ~/.codex/sessions and were in NO signature, + so a Codex workspace prints "Archived 1 Codex transcript(s)" and then "Up to + date" in the same run, and the transcript is never parsed. Signing the + outside source directories cannot see this, because Codex never passes + through them. + """ + from synapt.recall import codex as codex_mod + from synapt.recall.cli import _archive_and_build + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + sessions = tmp_path / "codex-sessions" / "2026" / "03" / "01" + _codex_rollout(sessions, project, "first", "about the first codex thing") + + real_archive = codex_mod.archive_codex_transcripts + monkeypatch.setattr( + codex_mod, "archive_codex_transcripts", + lambda project_dir, sessions_dir=None: real_archive( + project_dir, sessions_dir=tmp_path / "codex-sessions"), + ) + + first = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + assert any("about the first codex thing" in _chunk_text(c) for c in first.chunks), ( + "the Codex control never landed; this witness would prove nothing" + ) + capsys.readouterr() + + _codex_rollout(sessions, project, "second", "CODEXLATESENTINEL") + + index = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + out = capsys.readouterr().out + + assert "up to date" not in out.lower(), ( + f"a new Codex transcript was archived and the build called itself up to date:\n{out}" + ) + hits = [c for c in index.chunks if "CODEXLATESENTINEL" in _chunk_text(c)] + assert hits, f"a new Codex transcript reached the archive and was never parsed:\n{out}" + + +def test_a_source_write_after_the_archive_copy_is_not_certified(tmp_path, capsys, monkeypatch): + """Step 1 copies, then the signature samples. The gap between is real. + + A transcript landing after the copy but before the sample is absent from the + archive the build parses and present in a signature taken over the source + directories, so it is persisted as certified. The next run copies it into the + archive and then skips, because the source side has not moved since. + """ + from synapt.recall import archive as archive_mod + from synapt.recall.cli import _archive_and_build + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + state = {"fired": False} + real_archive_transcripts = archive_mod.archive_transcripts + + def archive_then_a_source_write(*args, **kwargs): + result = real_archive_transcripts(*args, **kwargs) + if not state["fired"]: + state["fired"] = True + _transcript(source / "postcopy.jsonl", turns=6, prefix="POSTCOPYSENTINEL") + return result + + monkeypatch.setattr(archive_mod, "archive_transcripts", archive_then_a_source_write) + _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + monkeypatch.undo() + assert state["fired"], "the post-copy write never fired; this witness proved nothing" + capsys.readouterr() + + index = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + out = capsys.readouterr().out + + hits = [c for c in index.chunks if "POSTCOPYSENTINEL" in _chunk_text(c)] + assert hits, ( + "a transcript written after the archive copy was signed as certified, so " + f"the next run copied it into the archive and then skipped parsing it:\n{out}" + ) + + +def test_a_channel_message_arriving_before_the_read_boundary_is_not_certified( + tmp_path, monkeypatch +): + """The start-to-read-boundary comparison is load-bearing, and witnessed. + + Found by mutation twice: it originally guarded mid-parse transcript + arrivals, and keying the signature on `build_sources` subsumed that case, + so the mutation stopped reddening anything. Its remaining responsibility is + channels and the archive, parsed BEFORE the journal read boundary. The + arrival fires from stub synthesis, which runs after channel parsing and + before that boundary -- squarely inside the window. + + Store resolution is pinned through the explicit env seam rather than + inferred from cwd. An earlier attempt at this witness resolved the channels + directory by inference and was defeated by it; that is the store-resolution + class, not this guard, and the fix is to stop inferring. (Atlas, r2 on v7.) + """ + from synapt.recall import journal as journal_mod + from synapt.recall.cli import _archive_and_build + + project = tmp_path / "proj" + project.mkdir() + source = tmp_path / "source" + source.mkdir() + _transcript(source / "s1.jsonl", turns=8) + + ch = tmp_path / "channels" + ch.mkdir() + monkeypatch.setenv("SYNAPT_SHARED_CHANNELS_DIR", str(ch)) + (ch / "dev.jsonl").write_text(json.dumps({ + "id": "m1", "timestamp": "2026-03-01T09:00:00Z", "channel": "dev", + "type": "message", "body": "the first channel message", "from": "a", + }) + "\n") + + state = {"fired": False} + real_synth = journal_mod.synthesize_journal_stubs + + def synthesize_then_a_channel_arrives(*args, **kwargs): + result = real_synth(*args, **kwargs) + if not state["fired"]: + state["fired"] = True + (ch / "late.jsonl").write_text(json.dumps({ + "id": "m2", "timestamp": "2026-03-01T09:30:00Z", "channel": "late", + "type": "message", "body": "CHANNELMIDPARSESENTINEL", "from": "b", + }) + "\n") + return result + + monkeypatch.setattr(journal_mod, "synthesize_journal_stubs", synthesize_then_a_channel_arrives) + first = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + monkeypatch.setattr(journal_mod, "synthesize_journal_stubs", real_synth) + + assert state["fired"], "the channel arrival never fired; this witness proved nothing" + assert any(c.session_id.startswith("channel_") for c in first.chunks), ( + "channels were never indexed at all; this witness would prove nothing" + ) + assert not [c for c in first.chunks if "CHANNELMIDPARSESENTINEL" in _chunk_text(c)], ( + "the late channel file was parsed by the run that created it; the arrival " + "did not land inside the intended window" + ) + + index = _archive_and_build(project, source_dirs=[source], use_embeddings=False, incremental=True) + + hits = [c for c in index.chunks if "CHANNELMIDPARSESENTINEL" in _chunk_text(c)] + assert hits, ( + "a channel message that arrived after channels were parsed was sampled " + "into the read-boundary signature it never reached the index through" + ) From 9b25e42e902820d5f9538ed2379e2e9ebbb7955b Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Wed, 19 Aug 2026 04:29:44 -0500 Subject: [PATCH 17/19] fix(journal): normalize timestamp ordering --- src/synapt/recall/journal.py | 32 +++++++++++--- tests/recall/test_journal.py | 82 ++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/src/synapt/recall/journal.py b/src/synapt/recall/journal.py index d75ab26d..32ee6201 100644 --- a/src/synapt/recall/journal.py +++ b/src/synapt/recall/journal.py @@ -303,7 +303,7 @@ def read_entries(path: Path | None = None, n: int = 5) -> list[JournalEntry]: return [] raw = _read_all_entries(path) deduped = _dedup_entries(raw) - deduped.sort(key=lambda e: e.timestamp, reverse=True) + deduped.sort(key=lambda e: _timestamp_order(e.timestamp), reverse=True) return deduped[:n] @@ -331,14 +331,36 @@ def _entry_richness(entry: JournalEntry) -> tuple: hides the current entry's next steps, which are the continuity handoff. Returns a tuple that sorts higher for the retained entry: - (not auto, timestamp, rich field count). + (not auto, normalized timestamp, rich field count). """ rich_count = sum(bool(f) for f in (entry.focus, entry.done, entry.decisions, entry.next_steps)) - return (not entry.auto, entry.timestamp, rich_count) + return (not entry.auto, _timestamp_order(entry.timestamp), rich_count) + + +def _timestamp_order(timestamp: str) -> datetime: + """Return a deterministic chronological ordering key for a journal timestamp. + + Offset-aware values are normalized to UTC. Legacy offset-naive values are + interpreted as UTC because their original timezone is not recoverable from + disk. An unparseable legacy value sorts before any parseable timestamp so + it cannot displace a known newer journal entry. + """ + if timestamp.endswith("Z"): + timestamp = f"{timestamp[:-1]}+00:00" + try: + parsed = datetime.fromisoformat(timestamp) + except ValueError: + return datetime.min.replace(tzinfo=timezone.utc) + if parsed.tzinfo is None: + return parsed.replace(tzinfo=timezone.utc) + try: + return parsed.astimezone(timezone.utc) + except OverflowError: + return datetime.min.replace(tzinfo=timezone.utc) def _dedup_entries(entries: list[JournalEntry]) -> list[JournalEntry]: - """Keep only the richest entry per session_id. + """Keep the highest-priority entry per session_id. Priority: non-auto > auto, then newest timestamp, then most rich fields (focus/done/decisions/next_steps). @@ -405,7 +427,7 @@ def compact_journal(path: Path | None = None) -> int: removed = len(entries) - len(deduped) if removed == 0: return 0 - deduped.sort(key=lambda e: e.timestamp) # chronological for storage + deduped.sort(key=lambda e: _timestamp_order(e.timestamp)) f.seek(0) f.truncate(0) # explicit arg: truncate to zero bytes regardless of buffer position for entry in deduped: diff --git a/tests/recall/test_journal.py b/tests/recall/test_journal.py index 58dd3137..36331daa 100644 --- a/tests/recall/test_journal.py +++ b/tests/recall/test_journal.py @@ -336,6 +336,88 @@ def test_read_entries_keeps_newer_current_next_steps_over_richer_prior_entry(sel self.assertEqual(latest.next_steps, ["current next step"]) self.assertNotIn("completed item", latest.done) + def test_dedup_orders_mixed_timestamp_formats_chronologically(self): + """A naive legacy timestamp is UTC, not a lexicographic timestamp.""" + append_entry(JournalEntry( + timestamp="2026-08-13T12:00:00", session_id="resumed-session", + focus="older naive entry", auto=False, + ), self.path) + append_entry(JournalEntry( + timestamp="2026-08-13T11:30:00-02:00", session_id="resumed-session", + focus="newer aware entry", auto=False, + ), self.path) + + latest = read_latest(self.path) + + self.assertIsNotNone(latest) + self.assertEqual(latest.focus, "newer aware entry") + + def test_read_and_compact_order_mixed_timestamp_formats_chronologically(self): + """All journal ordering uses the same timestamp interpretation.""" + append_entry(JournalEntry( + timestamp="2026-08-13T12:00:00", session_id="naive", + focus="older naive entry", auto=False, + ), self.path) + append_entry(JournalEntry( + timestamp="2026-08-13T11:30:00-02:00", session_id="aware", + focus="newer aware entry", auto=False, + ), self.path) + append_entry(JournalEntry( + timestamp="2026-08-13T10:00:00+00:00", session_id="dedup", + focus="discarded auto entry", auto=True, + ), self.path) + append_entry(JournalEntry( + timestamp="2026-08-13T10:30:00+00:00", session_id="dedup", + focus="dedup winner", auto=False, + ), self.path) + + self.assertEqual( + [entry.focus for entry in read_entries(self.path, n=2)], + ["newer aware entry", "older naive entry"], + ) + + self.assertEqual(compact_journal(self.path), 1) + with open(self.path) as f: + stored = [json.loads(line)["focus"] for line in f if line.strip()] + self.assertEqual( + stored, + ["dedup winner", "older naive entry", "newer aware entry"], + ) + + def test_unparseable_timestamp_sorts_before_known_timestamps(self): + """A malformed legacy timestamp cannot displace a dated entry.""" + append_entry(JournalEntry( + timestamp="not-a-timestamp", session_id="unknown", focus="unknown time", + ), self.path) + append_entry(JournalEntry( + timestamp="2026-08-13T10:00:00+00:00", session_id="known", focus="known time", + ), self.path) + + self.assertEqual( + [entry.focus for entry in read_entries(self.path, n=2)], + ["known time", "unknown time"], + ) + + def test_extreme_offset_timestamp_sorts_before_known_timestamps(self): + """A parseable legacy value that overflows UTC conversion stays readable.""" + append_entry(JournalEntry( + timestamp="0001-01-01T00:00:00+05:00", session_id="underflow", + focus="underflow time", + ), self.path) + append_entry(JournalEntry( + timestamp="9999-12-31T23:59:59-05:00", session_id="overflow", + focus="overflow time", + ), self.path) + append_entry(JournalEntry( + timestamp="2026-08-13T10:00:00+00:00", session_id="known", + focus="known time", + ), self.path) + + entries = read_entries(self.path, n=3) + + self.assertEqual(entries[0].focus, "known time") + self.assertEqual({entry.focus for entry in entries[1:]}, {"underflow time", "overflow time"}) + def test_compact_journal_removes_duplicates(self): """compact_journal deduplicates and sorts the file.""" append_entry(JournalEntry( From c94ef1a9e25f672898f83572798f586bf6c776e3 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Fri, 21 Aug 2026 04:43:03 -0500 Subject: [PATCH 18/19] fix(hook): stop announcing a resume the hook never performs The Claude session-start prompt opened with "SessionStart:resume hook success" while performing no resume, and instructed a cron-backed monitoring loop that is no longer the intended pattern. The two halves were invisible for different reasons, and only one was defended. test_startup.py asserted `"CronCreate for the loop" in prompt` under the docstring "Claude agents keep the existing cron-backed monitoring instruction" -- so the suite reported agreement with an instruction that had already been withdrawn. A green test pinning a retired instruction is worse than no test: it looks like corroboration. The false label was not defended, it was UNCHECKED: no assertion referenced it in either direction, so it simply survived. A thing a passing test does not contradict is not a thing that test defends. The label now states what the hook DID (startup context loaded) rather than naming a mechanism that does not run, and the prompt tells the agent to wait to be prompted and notify its coordinator on completion instead of polling on a cadence. Each half is mutation-proven separately, because stacked asserts short-circuit and one mutation would only have proven the first layer. Premium boundary: recall is OSS -- this is session-start context surfacing, no identity or org semantics. --- src/synapt/recall/cli.py | 27 ++++++++++++++++++--------- tests/recall/test_startup.py | 26 ++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/src/synapt/recall/cli.py b/src/synapt/recall/cli.py index 522e2523..6c92e761 100644 --- a/src/synapt/recall/cli.py +++ b/src/synapt/recall/cli.py @@ -2385,15 +2385,23 @@ def _dev_loop_activation_prompt(project: Path) -> str | None: f"Use the startup context to choose the next concrete task." ) - interval = ( - os.environ.get("SYNAPT_LOOP_INTERVAL") - or str(agent_cfg.get("loop_interval") or "2m") - ) + # No resume is performed here, so the label does not claim one. The prior + # text read "SessionStart:resume hook success", which announced a handoff + # the hook never carried out -- a label is not a mechanism, and this one + # survived truncation more reliably than the context it mislabelled. + # + # The monitoring loop is likewise removed rather than reworded: a + # cadence-poll spends context re-reading unchanged state, and an agent + # that waits to be prompted does not need one. + # Claude CAN self-wake where Codex cannot, which is why this instruction + # had to be retired deliberately instead of falling away on its own. return ( - f"SessionStart:resume hook success: Agent {agent_name} — join #{channel} " - f"and start monitoring loop at {interval} interval. Use recall_channel to " - f"join, CronCreate for the loop. Use show_pins=false and detail=medium for " - f"polling. Prefer doing needed work over reporting that work exists." + f"SessionStart:startup context loaded: Agent {agent_name} — join " + f"#{channel} once with recall_channel if channel context is needed. " + f"Do NOT create a cron loop or poll on a cadence: the monitoring " + f"loop is deprecated. Wait to be prompted, and notify your " + f"coordinator when a task completes. Prefer doing needed work over " + f"reporting that work exists." ) @@ -2511,7 +2519,8 @@ def cmd_hook(args: argparse.Namespace) -> None: # 10. Dev-loop activation prompt — deterministic hook replaces # unreliable skill auto-activation (~20%). The agent reads this - # system reminder and follows the instructions to start monitoring. + # system reminder as its startup instruction; it does not start + # a monitoring loop. try: prompt = _dev_loop_activation_prompt(project) if prompt: diff --git a/tests/recall/test_startup.py b/tests/recall/test_startup.py index 07f5bca3..4f1dba8c 100644 --- a/tests/recall/test_startup.py +++ b/tests/recall/test_startup.py @@ -239,8 +239,20 @@ def test_gripspace_root_env_resolves_sibling_griptree_config(self, tmp_path): assert "deprecated for Codex" in prompt assert "CronCreate for the loop" not in prompt - def test_claude_prompt_keeps_croncreate_instruction(self, tmp_path): - """Claude agents keep the existing cron-backed monitoring instruction.""" + def test_claude_prompt_claims_no_resume_and_no_loop(self, tmp_path): + """Claude agents are told what the hook DID, and not to poll. + + This test previously asserted the opposite -- it pinned + "CronCreate for the loop" as required output, so the deprecated + monitoring loop was defended by a passing test after the + instruction had already been withdrawn. A green suite asserting + behaviour that was retired is worse than an untested one: + it reports agreement. + + The label is checked too. The prompt used to open + "SessionStart:resume hook success" while performing no resume, + which is a claim about a mechanism that does not run. + """ project = tmp_path / "worktree" project.mkdir() gitgrip = tmp_path / ".gitgrip" @@ -261,8 +273,14 @@ def test_claude_prompt_keeps_croncreate_instruction(self, tmp_path): prompt = _dev_loop_activation_prompt(project) assert prompt is not None - assert "CronCreate for the loop" in prompt - assert "5m interval" in prompt + # The label must not announce a resume the hook never performs. + assert "SessionStart:resume hook success" not in prompt + assert "startup context loaded" in prompt + # The deprecated loop must not be instructed, in any of its forms. + assert "CronCreate" not in prompt + assert "5m interval" not in prompt + assert "monitoring loop is deprecated" in prompt + assert "notify your coordinator" in prompt class TestStartupSubcommand: From d178530881bb0e7c10bb624d1b98565c5f5c4b50 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Fri, 21 Aug 2026 19:03:24 -0500 Subject: [PATCH 19/19] chore: release 0.19.0 Version bump, a changelog entry, and one bug fix that belongs in the release rather than after it. WHY 0.19.0 AND NOT 0.18.1. 0.18.0 was published to PyPI at 2026-08-07T18:01:21Z. All 17 first-parent units in the range ca62880..bc594f3 landed after that instant, checked against each commit's date rather than inferred, so the published artifact contains none of them. PyPI forbids re-uploading a version. An earlier version of this message said 11 of those units landed after the publish date. That was wrong. 11 is a real count, of units carrying product changes, attached to a claim about timing where the answer is all 17. The range starts at the 0.18.0 promotion merge, four minutes before PyPI published, so no subset of it can be the post-publish set. Caught in review before anything shipped. COUNTING RULE. Over ca62880..bc594f3: 35 commits, 17 merges, 18 direct, 17 first-parent. Of the 17, twelve touch src/ and five touch only tests, docs, or fixtures; one of those five is a topology-repair merge carrying no content. THE FIX. tests/recall_store_isolation.py imported the POSIX-only pwd module at module scope. conftest.py imports that module at root scope, so on Windows the ModuleNotFoundError fired before any collection at all, taking down the whole run rather than one module. The import now happens at call time; POSIX behaviour is byte-identical. Deliberately not replaced with Path.home(). That reads $HOME, which is exactly the value a fixture can move, and the protected boundary must not be derivable from the value under test or the guarantee becomes circular. On Windows it still raises, at the point of use, which is honest for a POSIX-only guarantee. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 85 +++++++++++++++++++++++++++++++++ pyproject.toml | 2 +- src/synapt/recall/__init__.py | 2 +- tests/recall_store_isolation.py | 14 +++++- 4 files changed, 100 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05ab3f86..80253209 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,91 @@ All notable changes to synapt are documented here. +## [0.19.0] — 2026-08-21 + +**Counting rule, stated so it can be checked.** Range `ca62880..bc594f3` — from the 0.18.0 +promotion merge to the `dev` tip at this release. Over that range: **35 commits total, 17 of +them merge commits and 18 direct. 17 first-parent commits.** + +Composition of those 17, by path: **12 touch `src/` when each unit is diffed against its first +parent**; 5 touch only tests, docs, or fixtures; and one of those 5 (`116ffcc`) is a +post-promotion topology-repair merge carrying no content by its own message. "Reviewed units +landed" is therefore 16 if you mean units that changed something, and 17 if you mean merges in +the range. + +**The diff method is part of the claim, not pedantry.** A merge commit has no canonical diff. +Against first parent — or `git log --first-parent --diff-merges=first-parent` — the answer is +12. But `git show --name-only`, which is the command most readers will reach for first, +returns **0**: a combined diff omits everything that was not a conflict resolution, and it does +so silently rather than as an error. A count offered for checking is worth only as much as the +method that reproduces it. + +**Why 0.19.0 and not 0.18.1.** 0.18.0 was published to PyPI at `2026-08-07T18:01:21Z`. **Every +one of the 17 units above landed after that instant** — measured against each commit's date, +not inferred. The published 0.18.0 artifact contains none of this work. PyPI forbids +re-uploading a version, so this is a new minor rather than a patch to the existing one. + +An earlier draft of this section said "11 of the units above landed after that date." **The +number 11 was real but attached to the wrong category** — it counts units carrying product +changes, which the prose never named, while the sentence was making a claim about *timing*, +where the true answer is all 17. Caught in review before this shipped. Worth recording rather than silently +correcting, because this section's heading invites a reader to check the numbers, and a reader +who finds one that fails has no way to know the other four are exact. + +### Fixed +- **Windows test collection, broken on every pull request since 2026-08-07.** + `tests/recall_store_isolation.py` imported the POSIX-only `pwd` module at module scope, and + `conftest.py` imports that module at *root* scope. The `ModuleNotFoundError` therefore fired + before any collection at all — earlier than skip markers exist, so no marker could have + applied — taking down the entire Windows run rather than one module. The import is now made + at call time. POSIX behaviour is byte-identical. + + **What is recorded here is the fact, without a theory attached.** Across all 19 pull-request + runs since 0.18.0 published, three Windows jobs failed in **every single one**, without + exception. That is measured, not sampled. + + Why it went unaddressed for two weeks is **not something this entry can answer honestly.** An + earlier draft offered an explanation — that the rest of the matrix stayed green, so a + partially-red result read as a flaky lane rather than a broken one. Measurement does not + support it: that pattern holds in 8 of the 19 runs and is false in the other 11, where the + entire matrix was red for reasons this fix does not address and this entry does not diagnose. + The explanation was removed rather than softened. + + Deliberately **not** replaced with `Path.home()`: that function reads `$HOME`, which is + precisely the value a test fixture can move, and the protected boundary must not be + derivable from the value under test or the guarantee becomes circular. On Windows the + function still raises, loudly and at the point of use, which is the honest outcome for a + POSIX-only guarantee. + +### Added +- **Incremental builds by default**, with a new `maintain` command and change-detection + idempotence. + +### Changed +- **Summary work moved out of `build` and into `maintain`.** This is a user-visible behaviour + change, not only an internal one: a `build` that previously produced summaries no longer + does, and `maintain` is where that work now happens. + +### Improved +- **Store and data-root isolation**, including a resolution fix so that membership takes + precedence over locality. +- **Journal correctness**, and a root-resolution fix across the archive verbs — export and + import, and also the archive, CLI, and server paths. +- **`code_git` hardening** and a session-start prompt fix. + +### Documentation +- Fixture provenance is now declared for the identify test fixtures. + +### Note on the changelog gap +**0.15.2, 0.15.3, 0.16.0, 0.17.0, and 0.18.0 shipped without changelog entries** — the gap is +wider than the three versions an earlier draft named. Rather than reconstruct them after the +fact from commit archaeology, it is recorded here honestly. + +Their content **is** recoverable from the git history between the corresponding tags. **All five +were checked on the public remote** with `ls-remote` — `v0.15.2`, `v0.15.3`, `v0.16.0`, +`v0.17.0`, and `v0.18.0` — not merely the three an earlier draft vouched for while naming five. +A recovery instruction is worth only as much as the refs it names. + ## [0.15.1] — 2026-05-12 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 521fac60..f9551a08 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "synapt" -version = "0.18.0" +version = "0.19.0" description = "Persistent conversational memory for AI coding assistants" readme = "README.md" license = "MIT" diff --git a/src/synapt/recall/__init__.py b/src/synapt/recall/__init__.py index 6269df13..9140704a 100644 --- a/src/synapt/recall/__init__.py +++ b/src/synapt/recall/__init__.py @@ -1,6 +1,6 @@ """synapt.recall — persistent conversational memory for Claude Code and ChatGPT sessions.""" -__version__ = "0.18.0" +__version__ = "0.19.0" from synapt.recall.core import ( TranscriptChunk, diff --git a/tests/recall_store_isolation.py b/tests/recall_store_isolation.py index 747ee3b7..7b927f0b 100644 --- a/tests/recall_store_isolation.py +++ b/tests/recall_store_isolation.py @@ -15,7 +15,6 @@ from __future__ import annotations import os -import pwd from pathlib import Path @@ -35,7 +34,20 @@ def account_home() -> Path: precisely because a fixture can move both of those. The protected boundary must not be derivable from the value a test is currently asking the code to use, or every guarantee here becomes circular. + + ``pwd`` is imported here rather than at module scope because it is POSIX-only. + At module scope it raises ``ModuleNotFoundError`` on Windows during pytest + *collection* — before any skip marker could apply — which took down the whole + Windows run rather than this one module. Importing at call time keeps + collection working everywhere and leaves POSIX behaviour byte-identical. + + Deliberately NOT replaced with ``Path.home()``: that reads ``$HOME``, which is + exactly the value a fixture can move, and would make the boundary circular in + the way the paragraph above forbids. On Windows this still raises, loudly and + at the point of use, which is the honest outcome for a POSIX-only guarantee. """ + import pwd + return Path(pwd.getpwuid(os.getuid()).pw_dir).resolve()