refactor(tests): split conftest.py under the 300-line cap (boy-scout follow-up)

tests_py/_domain_mapping_isolation.py

@@ -0,0 +1,61 @@
+"""Isolate `shared.domain_mapping`'s dev-root scan from the real filesystem.
+
+Extracted from `tests_py/conftest.py` (issue #276/#287 boy-scout follow-up
+— see `tests_py/_pg_throwaway_db.py`'s docstring for the size-cap
+rationale this split serves). No behavior change: `conftest.py` calls
+`isolate_dev_root_scan()` at the exact point in its module-level sequence
+where this code used to run inline.
+
+ROOT CAUSE (reproduced deterministically while measuring coverage for
+issue #196, 2026-07-27): `shared/domain_mapping._build_registry()` (used
+transitively by `handlers/consolidation/wiki_backlog_pass.py` ->
+`core/wiki_drift.py`'s `audit_wiki_drift` -> `_project_source_root` ->
+`_file_exists_under`'s `os.walk` fallback) has NO test override by
+default. `_candidate_dev_roots()` falls back to `$HOME/Developments`,
+`$HOME/Documents/Developments`, `$HOME/dev`, `$HOME/code` — real developer
+directories — and (this is the part that defeats a naive fix) is
+ADDITIVE: even with `$CORTEX_DEV_ROOT` set, every one of those defaults
+that exists on disk is STILL appended as an extra candidate (see
+`shared/domain_mapping.py::_candidate_dev_roots` — the env var is one more
+candidate, not a replacement). Setting the env var alone does NOT isolate
+the scan. On a machine where one of the defaults exists and holds real git
+repos (any contributor's normal dev machine), any test that exercises the
+full consolidate handler (e.g.
+`test_consolidate_stage_exception_does_not_crash_siblings`) transitively
+walks those REAL, unrelated, potentially enormous repo trees via
+`os.walk` — observed killing a `pytest --cov` run at the 300s per-test
+timeout (`pyproject.toml`'s `timeout = 300`). This is invisible in CI
+because none of the candidate dev roots exist on a CI runner
+(`$HOME=/home/runner`), so `_candidate_dev_roots()` returns `[]` there and
+`_project_source_root` always returns `None` — CI's isolation is
+accidental, not designed. This plausibly explains the previously
+unexplained "CI stall on 148d5a1... hung >3h... no offender identified"
+that same `timeout` setting was added to catch: a local run with a
+populated dev root would reproduce this hang with no per-test timeout to
+bound it.
+
+Fix: replace `_candidate_dev_roots` itself (not just the env var) so it
+always returns an empty list for the whole test session, matching the
+exact isolation pattern `tests_py/shared/test_domain_mapping.py` already
+uses per-test (monkeypatch `_candidate_dev_roots` directly, then
+`_build_registry.cache_clear()`). Those dedicated tests monkeypatch this
+same attribute again inside their own test body — monkeypatch always
+restores to whatever was in place when the test started, so patching it
+here at collection time (not via the `monkeypatch` fixture, which only
+exists inside a running test) is a permanent module-level replacement for
+the whole session; the per-test monkeypatch calls inside
+`test_domain_mapping.py` override it for the duration of those specific
+tests and their own `_build_registry.cache_clear()` calls put the real
+cache state back in sync afterward.
+"""
+
+from __future__ import annotations
+
+
+def isolate_dev_root_scan() -> None:
+    """Permanently replace `_candidate_dev_roots` with an empty-list stub
+    for the whole test session (see module docstring for why)."""
+    from mcp_server.shared import domain_mapping as dm
+
+    dm._candidate_dev_roots = lambda: []
+    dm._build_registry.cache_clear()

tests_py/_pg_safety_guards.py

@@ -0,0 +1,125 @@
+"""Fail-closed guards against running the destructive test suite against a
+real database or a real `~/.claude` tree.
+
+Extracted from `tests_py/conftest.py` (issue #276/#287 boy-scout follow-up)
+as part of bringing that file under the repo's 300-line cap
+(coding-standards.md §4.1) — see `tests_py/_pg_throwaway_db.py`'s docstring
+for the full rationale. Behavior is unchanged: both guards below still
+call `pytest.exit(..., returncode=2)` themselves (that call does not need
+to happen inside a named pytest hook — conftest.py invokes these as plain
+functions at module-import time, exactly as it invoked the code before the
+extraction), with the same messages and the same conditions. The only
+change is that they take their inputs as explicit parameters instead of
+reading `tests_py/conftest.py`'s module globals directly — an incidental
+DI improvement (these are independently testable now), not the goal.
+"""
+
+from __future__ import annotations
+
+import os
+
+import pytest
+
+
+def looks_like_test_db(url: str) -> bool:
+    """True when the database NAME declares itself a test/bench database.
+
+    `.rsplit("/", 1)[-1]` and `.split("?", 1)[0]` are both selected for
+    their FIRST/LAST-element indexing, not their maxsplit count: `[-1]`
+    from `rsplit(sep, N)` is the same for every N>=1 or unlimited (it is
+    always "everything after the rightmost separator"), and `[0]` from
+    `split(sep, N)` is likewise invariant to N (always "everything before
+    the leftmost separator"). Only swapping `rsplit` for `split` (or vice
+    versa) on the SAME call changes the result, and only when 2+
+    occurrences of that separator are present.
+    """
+    name = url.rsplit("/", 1)[-1].split("?", 1)[0]
+    return name.endswith("_test") or name.endswith("_bench") or "_test_" in name
+
+
+def guard_against_populated_db(test_db_url: str) -> None:
+    """Refuse to run destructive test isolation against a populated DB.
+
+    INCIDENT 2026-06-10: an agent ran ``DATABASE_URL=<production> CI=true
+    pytest`` — the CI branch trusted the URL verbatim and
+    ``_clean_all_tables`` deleted 537,396 production memories (plus all
+    entities, relationships, and prospective triggers) between tests.
+
+    Two-tier guard, fail-closed:
+      * a database whose NAME marks it as a test DB (``*_test`` /
+        ``*_bench``) is trusted — local cortex_test and bench DBs;
+      * any OTHER name (e.g. the fresh runner-local ``cortex`` that real
+        CI creates) must hold ZERO memories at session start. A genuine
+        test database is created empty; pre-existing rows mean this is
+        somebody's real store. No threshold constant — empty is empty.
+
+    Override (explicit, never default): CORTEX_TEST_ALLOW_POPULATED=1.
+    """
+    if os.environ.get("CORTEX_TEST_ALLOW_POPULATED") == "1":
+        return
+    if looks_like_test_db(test_db_url):
+        return
+    try:
+        import psycopg
+
+        with psycopg.connect(test_db_url, autocommit=True, connect_timeout=3) as conn:
+            row = conn.execute("SELECT COUNT(*) FROM memories").fetchone()
+            count = row[0] if row else 0
+    except Exception:
+        return  # unreachable/uninitialized DB — the SQLite fallback handles it
+    if count > 0:
+        pytest.exit(
+            f"REFUSING to run: test DB '{test_db_url}' is not named like a "
+            f"test database and already holds {count} memories. The test "
+            "suite DELETEs every table between tests — pointing it at a real "
+            "store destroys it (incident 2026-06-10: 537k rows lost). Use a "
+            "*_test database, or set CORTEX_TEST_ALLOW_POPULATED=1 if you "
+            "really mean it.",
+            returncode=2,
+        )
+
+
+def _resolved_real_data_roots() -> list[tuple[str, str]]:
+    """`(name, resolved value)` for every root that must stay inside the
+    isolated tree — read fresh from `mcp_server`, never from the env vars
+    `conftest.py`'s `_redirect_real_data_roots` set: an import that happened
+    too early, or a future edit that moves the redirection below the first
+    import, would leave these constants bound to the real `~/.claude`
+    while the env vars still look correct."""
+    from mcp_server.infrastructure.config import CLAUDE_DIR, WIKI_ROOT
+    from mcp_server.infrastructure.memory_config import get_memory_settings
+
+    settings = get_memory_settings()
+    return [
+        ("CLAUDE_DIR", str(CLAUDE_DIR)),
+        ("WIKI_ROOT", str(WIKI_ROOT)),
+        ("MemorySettings.DB_PATH", settings.DB_PATH),
+        ("MemorySettings.SQLITE_FALLBACK_PATH", settings.SQLITE_FALLBACK_PATH),
+    ]
+
+
+# Fail-closed and unconditional: there is no override. A suite that DELETEs
+# every table between tests and rewrites the wiki has no safe way to run
+# against a real tree (issue #219 — the previous isolation was conditional
+# and its failure was silent for as long as it took to notice a 0-row store).
+def guard_against_real_data_roots(isolated_claude_dir: str) -> None:
+    """Refuse to run if any resolved root still points at real user data."""
+    isolated = os.path.realpath(isolated_claude_dir)
+    offenders = [
+        (name, value)
+        for name, value in _resolved_real_data_roots()
+        if not os.path.realpath(os.path.expanduser(value)).startswith(isolated)
+    ]
+    if not offenders:
+        return
+    detail = "\n".join(f"    {name} -> {value}" for name, value in offenders)
+    real_root = os.path.realpath(os.path.expanduser("~/.claude"))
+    pytest.exit(
+        "REFUSING to run: test isolation is not in effect. These roots "
+        f"still resolve outside the throwaway tree {isolated}:\n{detail}\n"
+        f"(real user root: {real_root}). The suite DELETEs every table "
+        "between tests and regenerates wiki dashboards — running it "
+        "against a real tree destroys data (issue #219). Ensure "
+        "_redirect_real_data_roots() runs before any mcp_server import.",
+        returncode=2,
+    )

tests_py/_pg_throwaway_db.py

@@ -0,0 +1,146 @@
+"""Per-process throwaway PostgreSQL test-database lifecycle.
+
+Extracted from `tests_py/conftest.py` (issue #276/#287 boy-scout follow-up):
+that file was 501 lines, over both the repo's local 300-line cap and the
+global 500-line cap (coding-standards.md §4.1), with these functions
+accounting for roughly a third of it. Split along the concern boundary that
+was already visible in the file's own section comments — no behavior
+change, only explicit-parameter functions in place of closures over
+conftest.py's module globals (a DI improvement as a side effect, not the
+goal: these are independently testable now).
+
+Ordering/behavior preserved exactly: `conftest.py` still creates the
+isolated database (if any) before running its data-safety guards, still
+drops it in `pytest_sessionfinish`, and every message/exception-swallowing
+choice below is unchanged from the code this was extracted from.
+
+Why per-process isolation exists at all
+----------------------------------------
+ROOT CAUSE (reproduced deterministically 3/3 + 3/3, 2026-07-10): every
+worktree/agent used to default to the SAME physical DB (``cortex_test``).
+``conftest.py``'s autouse ``_test_isolation`` fixture runs an unconditional
+``DELETE FROM <table>`` before/after EVERY test, in EVERY concurrently
+running pytest process. Two pytest invocations sharing that DB race:
+process A's between-test purge can delete the row process B just stored,
+between B's ``remember()`` and its later ``recall()``/``get_memory()``
+call. This produced two previously-reported flakes with unrelated
+proximate symptoms (``test_store_consolidate_recall``:
+``recall_result["count"] == 0``; ``test_memory_count_unchanged_after_validation``:
+``store.get_memory() is None``) — neither an intra-suite ordering bug nor
+an embeddings-backend issue, both inter-process contention on one shared
+table set. Retrying or skipping would hide the race, not fix it (forbidden
+anti-pattern). The correct fix is to give each local pytest process its
+own throwaway database — CI already gets a fresh service-container DB per
+run, and an explicit ``CORTEX_TEST_DATABASE_URL`` override is respected
+verbatim (the caller has taken responsibility for isolation).
+"""
+
+from __future__ import annotations
+
+import os
+
+
+def maintenance_url(url: str) -> str:
+    """Same host/creds as `url`, database swapped to the `postgres` maintenance DB."""
+    base, _, _query = url.partition("?")
+    prefix, _, _dbname = base.rpartition("/")
+    return f"{prefix}/postgres"
+
+
+def _orphan_pid(name: str) -> int | None:
+    """The pid encoded in a `cortex_test_pw<pid>_<8-hex>` database name, or
+    `None` when the name doesn't carry a numeric pid."""
+    rest = name[len("cortex_test_pw") :]
+    pid_str = rest.split("_", 1)[0]
+    return int(pid_str) if pid_str.isdigit() else None
+
+
+def _pid_is_definitely_dead(pid: int) -> bool:
+    """True only when `pid` is confirmed gone. Best-effort, non-fatal: every
+    ambiguous case (still alive, owned by another user, any other OS error)
+    counts as "alive" — never risk a false-positive drop of a database
+    still in use."""
+    try:
+        os.kill(pid, 0)
+        return False  # still alive
+    except ProcessLookupError:
+        return True
+    except Exception:
+        return False  # permission-denied or any other ambiguity — leave it
+
+
+def drop_dead_orphaned_databases(conn) -> None:
+    """Drop leftover `cortex_test_pw<pid>_<hex>` databases whose owning PID
+    is no longer alive.
+
+    A pytest process killed abnormally (SIGTERM/SIGKILL — e.g. a CI job
+    cancellation or a shell timeout) never reaches `pytest_sessionfinish`,
+    so its throwaway database is never dropped (measured: 1 orphan produced
+    by a bash-tool 2-minute timeout during this investigation, 2026-07-10).
+    """
+    try:
+        rows = conn.execute(
+            "SELECT datname FROM pg_database WHERE datname LIKE 'cortex_test_pw%';"
+        ).fetchall()
+    except Exception:
+        return
+    for row in rows:
+        name = row[0] if isinstance(row, tuple) else row.get("datname")
+        if not name:
+            continue
+        pid = _orphan_pid(name)
+        if pid is None or not _pid_is_definitely_dead(pid):
+            continue
+        try:
+            conn.execute(
+                "SELECT pg_terminate_backend(pid) FROM pg_stat_activity "
+                "WHERE datname = %s AND pid <> pg_backend_pid()",
+                (name,),
+            )
+            conn.execute(f'DROP DATABASE IF EXISTS "{name}"')
+        except Exception:
+            pass
+
+
+def create_isolated_test_database(base_url: str) -> tuple[str, str, str] | None:
+    """Create a throwaway PG database for this pytest process.
+
+    Returns `(isolated_url, maintenance_url, db_name)` on success, or `None`
+    on any failure (unreachable server, no CREATEDB privilege, etc.) so the
+    caller falls back to the shared `base_url` — degrading to prior
+    (contention-prone but functional) behavior rather than failing the
+    whole session.
+    """
+    try:
+        import psycopg
+
+        db_name = f"cortex_test_pw{os.getpid()}_{os.urandom(4).hex()}"
+        maint_url = maintenance_url(base_url)
+        with psycopg.connect(maint_url, autocommit=True, connect_timeout=3) as conn:
+            drop_dead_orphaned_databases(conn)
+            conn.execute(f'CREATE DATABASE "{db_name}"')
+    except Exception:
+        return None
+    prefix = base_url.rpartition("/")[0]
+    return f"{prefix}/{db_name}", maint_url, db_name
+
+
+def drop_isolated_database(maint_url: str, db_name: str) -> None:
+    """Drop the throwaway per-process database created above, if any.
+
+    Called from `conftest.py`'s `pytest_sessionfinish` hook (which must
+    stay a named hook function in conftest.py itself for pytest to find
+    it — this is the extracted body, not the hook).
+    """
+    try:
+        import psycopg
+
+        with psycopg.connect(maint_url, autocommit=True, connect_timeout=3) as conn:
+            conn.execute(
+                "SELECT pg_terminate_backend(pid) FROM pg_stat_activity "
+                "WHERE datname = %s AND pid <> pg_backend_pid()",
+                (db_name,),
+            )
+            conn.execute(f'DROP DATABASE IF EXISTS "{db_name}"')
+    except Exception:
+        pass