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/conftest.py b/conftest.py new file mode 100644 index 00000000..efd0c7e6 --- /dev/null +++ b/conftest.py @@ -0,0 +1,503 @@ +"""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=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=( + "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." + ), + ) + + +# --------------------------------------------------------------------------- +# 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 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 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 + + 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 + _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): + 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 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 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.""" + 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/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/src/synapt/recall/archive.py b/src/synapt/recall/archive.py index c5f2d75a..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 @@ -681,9 +739,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 +757,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 new file mode 100644 index 00000000..b8ec4780 --- /dev/null +++ b/src/synapt/recall/build_delta.py @@ -0,0 +1,243 @@ +"""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 +import re + +#: 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" + +#: 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: + """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, + archive_paths: Iterable[Path] | None = None, +) -> list[Path]: + """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 + 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) + + for archive in archive_paths or (): + archive = Path(archive) + if archive.is_file(): + files.append(archive) + + return files + + +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: 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 + 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, archive_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. + + 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 + 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. + + 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 _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/src/synapt/recall/channel.py b/src/synapt/recall/channel.py index d4ba0202..63575031 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: @@ -234,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" @@ -1080,6 +1127,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 +1310,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 +3126,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 +3192,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 @@ -3184,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/src/synapt/recall/cli.py b/src/synapt/recall/cli.py index 821a4c92..6c92e761 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(): @@ -561,14 +671,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: @@ -619,10 +753,77 @@ 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, }) - 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 @@ -1255,7 +1456,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, @@ -1267,7 +1471,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)} " @@ -1279,7 +1486,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( @@ -1295,7 +1503,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', '?')}" ) @@ -2176,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." ) @@ -2302,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: @@ -2756,17 +2974,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)", @@ -2787,7 +3072,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 @@ -2856,12 +3159,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") @@ -2978,6 +3283,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", @@ -2994,10 +3310,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/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/src/synapt/recall/core.py b/src/synapt/recall/core.py index c3638ff7..4f1523ca 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 @@ -4443,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``. @@ -4465,7 +4501,56 @@ 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 + + +# --------------------------------------------------------------------------- +# 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: @@ -4530,6 +4615,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" @@ -4844,10 +4934,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 @@ -4857,8 +4968,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/src/synapt/recall/journal.py b/src/synapt/recall/journal.py index 7b8f6122..32ee6201 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. """ @@ -302,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] @@ -324,18 +325,45 @@ 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, 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, rich_count, entry.timestamp) + 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 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 +381,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) @@ -398,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/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/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. 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 68f2feab..98fa7248 100644 --- a/tests/recall/test_action_registry.py +++ b/tests/recall/test_action_registry.py @@ -10,6 +10,10 @@ import unittest from unittest.mock import MagicMock, patch +from _isolation_helpers import owned_store + + + # OSS base actions that must always be available OSS_ACTIONS = { @@ -49,6 +53,12 @@ def test_registry_has_dispatch_method(self): class TestOSSBaseActions(unittest.TestCase): """OSS base actions must be registered by default.""" + def setUp(self): + self._store = owned_store() + + def tearDown(self): + self._store.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 +268,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._store = owned_store() + def tearDown(self): from synapt.recall.actions import reset_action_registry reset_action_registry() + 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_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" diff --git a/tests/recall/test_build_idempotence.py b/tests/recall/test_build_idempotence.py new file mode 100644 index 00000000..7a689b09 --- /dev/null +++ b/tests/recall/test_build_idempotence.py @@ -0,0 +1,1257 @@ +"""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 + + +@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}), + # 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}), + ("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 + + +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 +# =========================================================================== +# +# `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_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. + + 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" + ) + + +# =========================================================================== +# 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" + ) diff --git a/tests/recall/test_channel_hooks.py b/tests/recall/test_channel_hooks.py index 9641cf5b..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,8 +27,13 @@ class TestMessageHooks(unittest.TestCase): """Tests for the message hook registration and dispatch.""" def setUp(self): - self._tmp = tempfile.mkdtemp() - os.environ["SYNAPT_DATA_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) @@ -37,7 +42,7 @@ def tearDown(self): _clear_message_hooks() for hook in self._original_hooks: register_message_hook(hook) - os.environ.pop("SYNAPT_DATA_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_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" + ) diff --git a/tests/recall/test_codex.py b/tests/recall/test_codex.py index cc73e0c9..9d7d776c 100644 --- a/tests/recall/test_codex.py +++ b/tests/recall/test_codex.py @@ -3,6 +3,8 @@ import json import tempfile import unittest + +from _isolation_helpers import owned_store from pathlib import Path from synapt.recall.codex import ( @@ -572,6 +574,18 @@ 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 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): + self._store.restore() + 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 d5deea8d..e9d9722c 100644 --- a/tests/recall/test_dm_channels.py +++ b/tests/recall/test_dm_channels.py @@ -52,8 +52,20 @@ 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). + # 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_PROJECT_DIR": self.tmpdir, + "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), + "SYNAPT_RECALL_ROOT": str(recall_root), + "SYNAPT_RECALL_WORKTREE": "pytest-owned", }) self._env_patch.start() @@ -109,8 +121,20 @@ 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). + # 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_PROJECT_DIR": self.tmpdir, + "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), + "SYNAPT_RECALL_ROOT": str(recall_root), + "SYNAPT_RECALL_WORKTREE": "pytest-owned", }) self._env_patch.start() @@ -174,8 +198,20 @@ 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). + # 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_PROJECT_DIR": self.tmpdir, + "SYNAPT_SHARED_CHANNELS_DIR": str(Path(self.tmpdir) / "channels"), + "SYNAPT_RECALL_ROOT": str(recall_root), + "SYNAPT_RECALL_WORKTREE": "pytest-owned", }) self._env_patch.start() @@ -245,8 +281,20 @@ 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). + # 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_PROJECT_DIR": self.tmpdir, + "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_gripspace.py b/tests/recall/test_gripspace.py index 8a47d6f0..1658a1f8 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" @@ -281,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.""" diff --git a/tests/recall/test_journal.py b/tests/recall/test_journal.py index cb8555cb..36331daa 100644 --- a/tests/recall/test_journal.py +++ b/tests/recall/test_journal.py @@ -313,6 +313,111 @@ 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_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( @@ -406,14 +511,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, ), ] 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..4f1dba8c 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) @@ -238,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" @@ -260,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: diff --git a/tests/recall/test_store_isolation_guard.py b/tests/recall/test_store_isolation_guard.py new file mode 100644 index 00000000..4e2cc71b --- /dev/null +++ b/tests/recall/test_store_isolation_guard.py @@ -0,0 +1,773 @@ +"""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 +import sys +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_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. + + ``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 + + +# --------------------------------------------------------------------------- +# 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 +# --------------------------------------------------------------------------- + +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/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() diff --git a/tests/recall_store_isolation.py b/tests/recall_store_isolation.py new file mode 100644 index 00000000..7b927f0b --- /dev/null +++ b/tests/recall_store_isolation.py @@ -0,0 +1,227 @@ +"""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 +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. + + ``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() + + +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()))