fix(store,bench): a read-only open mode, so a diagnostic cannot mutate its subject (#1328)

CHANGELOG/v4.md

@@ -49,6 +49,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Two shipped diagnostics opened the live store read-write, so running one could flip a user's locks ([#1328](https://github.com/robotrocketscience/aelfrice/issues/1328)).** `MemoryStore(path)` is a *write* open: it runs the DDL battery, pending migrations, the `schema_meta` seed, `_resolve_local_scope_id` (which mints and persists a federation identity on a store that has none), eleven guarded one-shot passes, and — since [#1314](https://github.com/robotrocketscience/aelfrice/issues/1314) — `sweep_expired_locks`. `benchmarks/r3_idf_clip_bound.py` documents `--store .git/aelfrice/memory.db` and `benchmarks/temporal_spine_shadow.py` points `--db` at the same file, so an operator following either usage line mutated the corpus the script existed to measure. Reproduced on a copy of the live store: planting one expired time-boxed lock and then doing nothing but constructing a `MemoryStore` and closing it flipped `lock_level` from `user` to `none`, wrote a sweep marker, and changed the file bytes — with no analysis code having run. New `MemoryStore(..., read_only=True)` opens `mode=ro` and skips the entire open-time write window, so the engine refuses writes rather than the caller having to remember not to make them; the previous guarantee was a sentence in a benchmark docstring and it did not hold. `temporal_spine_shadow` keeps a write handle under `--backfill`, which is a deliberate write — the rule is *open for write only when writes were asked for*, not *never construct a store*. A test enumerates `benchmarks/*.py` from the directory and parses each with `ast`, failing on any `MemoryStore(...)` call without `read_only=` in a module that names a live-store path, because the failure mode is a **new** benchmark reaching for the convenient call — three did so independently. `benchmarks/consolidate_blocking_recall.py` was fixed the same way under review of [#1316](https://github.com/robotrocketscience/aelfrice/issues/1316).
 - **The consolidation audit reported zero duplicates exactly when the duplicate family was largest ([#1312](https://github.com/robotrocketscience/aelfrice/issues/1312)).** Candidate blocking skipped any 4-gram above a `df` cap, justified by the claim that genuine near-duplicates "share many rarer shingles as well, so they survive the cap through those." That is false for a *homogeneous* family: every member shares every shingle, so all of them sit at `df = K` and none is rarer. Past the cap the whole family lost every posting — at the shipped cap of 32, a 32-member family priced **31** removable and a 33-member family priced **0**. Beliefs the cap leaves with **no posting at all** are now rescued onto their shared shingles instead, so a homogeneous family is its own bucket at any size. The rescue is a *fallback*, not a replacement, and that distinction is the fix: an intermediate revision blocked every belief on its own rarest shingles, which closes the homogeneous cliff and opens a heterogeneous one — two near-duplicates whose minimum `df` differs never share a bucket, so the family shatters into components too small to report and the audit again reads **0** on a large family. That variant silently dropped 490 beliefs and 81 whole clusters the cap had found, including a 46-member clique in which all 1,035 pairs satisfy the shipped predicate. As a fallback the candidate set is a strict superset of the cap's, verified on the development store at 0 pairs, 0 clusters and 0 beliefs lost (`benchmarks/consolidate_blocking_recall.py`, which ships so the claim is re-derivable). On that store the largest cluster was being reported as 90 and is really **165**, 434 beliefs enter a cluster that previously entered none, and the published figure moves from 2.23% to **3.19% (1,424 of 44,594)** — the direction held, the number did not. What the fallback does *not* close is an over-cap family whose members each carry some low-`df` shingle to post to instead; measured against an uncapped pass that is 31 beliefs of 44,594, so the cap is kept as a tradeoff rather than removed. `n_beliefs_rescued` is reported, so the fallback is never silent, and the candidate budget now counts pairs *attempted* rather than distinct pairs kept — `candidates` is a set, so the old guard could never fire on the very boilerplate shape it was documented to bound. Also fixed alongside: `n_would_remove` counted user-locked members that `aelf retire` refuses without `--force`, and the O(k^2) medoid stage is now bounded by `MEDOID_SAMPLE_CAP` (74s → 46s on the development store, identical counts). `--consolidate-max-shingle-df` is replaced by `--consolidate-max-pairs`; the audit surface shipped unreleased, so no released flag changes.
 - **Opening a store while another process opened the same store could raise `database schema has changed` ([#1310](https://github.com/robotrocketscience/aelfrice/issues/1310)).** SQLite raises `SQLITE_SCHEMA` when the schema cookie moves between a statement's prepare and its step, which is exactly what two processes running the open-time `CREATE TABLE IF NOT EXISTS` battery against one `.git/aelfrice/memory.db` do to each other. `busy_timeout=5000` does not cover it — that pragma retries `database is locked`, a different error — so the constructor failed outright and took a required CI check down at random. The whole open-time window (the stale-`ingest_log` probe, `_SCHEMA`, `_MIGRATIONS`, the post-migration indexes, the `schema_meta` seed reads and writes, and the local scope-id resolve) now runs under one bounded re-prepare retry rather than only the DDL loop the traceback named: the reads are exposed to the same race, and a per-call-site patch would leave gaps as the constructor grows. The window is idempotent by construction (`IF NOT EXISTS` / `OR IGNORE` / marker-gated), so re-running it is a no-op. Only `schema has changed` is retried — every other `OperationalError` still propagates, and a persistently changing schema fails after a bounded number of attempts instead of spinning. Contrary to the issue's reasoning, the `_MIGRATIONS` catch three lines below did **not** already tolerate this class: it admits only `duplicate column name` and `no such column`, both of which are re-raised for anything else.
 - **`[relationship_detector]` was half-honoured at ingest: the flag took effect, the thresholds did not ([#1299](https://github.com/robotrocketscience/aelfrice/issues/1299)).** `ingest.py` resolved `auto_detect` from `.aelfrice.toml` and then called `write_semantic_edges(store, new_belief_ids=...)` with no threshold arguments, so `jaccard_min` / `confidence_min` / `max_candidate_pairs` reached the read-only `aelf doctor --relationships` audits and were silently ignored on the one path that actually mutates the graph. Adjacent keys in the same section, opposite reach, no trace on stderr — and the asymmetry ran the risky direction, with the audit tunable and the writer pinned at the module defaults. Ingest now threads the resolved config through. The three keys are parsed from the **same** `.aelfrice.toml` read that resolves `auto_detect` (new `resolve_ingest_relationship_config`), so the per-turn config-probe count is unchanged — measured 11 probes with no config file and 4 with one four directories up, before and after — rather than adding a second filesystem walk to a hot path ([#1289](https://github.com/robotrocketscience/aelfrice/issues/1289)/[#1298](https://github.com/robotrocketscience/aelfrice/issues/1298)). That measurement is env-unset; the env var is the other half of the precedence and it moves the count, so it is stated separately. `AELFRICE_AUTO_RELATIONSHIPS=0` decides the question without needing any threshold, and the flag-only resolver short-circuited on env before touching the filesystem — so that install paid **0** probes per turn and still does, the resolver returning before the walk rather than reading a config whose only consumer will not run. With the var set truthy the walk is real and new (0 -> 11 on a deep tree): the thresholds are then actually used, which is the point of the fix, so that one is a cost and not a regression. Precedence for `auto_detect` is unchanged (env > TOML > default-off), and default-off means a fresh install is byte-identical. `residual_overlap_min` and `max_edges_per_belief` still have no TOML key at all; `docs/user/CONFIG.md` now documents the section with per-key reach so which is which is readable.

benchmarks/r3_idf_clip_bound.py

@@ -359,7 +359,11 @@ def main(argv: list[str] | None = None) -> int:
         print("no user-turn prompts in the audit files", file=sys.stderr)
         return 2
 
-    store = MemoryStore(str(args.store))
+    # #1328: read-only. This script's own usage line points `--store` at
+    # `.git/aelfrice/memory.db`, and a bare open runs migrations plus the
+    # #1314 lock-expiry sweep — measuring a store is not a reason to
+    # mutate it.
+    store = MemoryStore(str(args.store), read_only=True)
     try:
         index = BM25Index.build(store)
     finally:

benchmarks/temporal_spine_shadow.py

@@ -422,7 +422,12 @@ def main(argv: Sequence[str] | None = None) -> int:
     ap.add_argument("--out", default="/tmp/temporal_spine_shadow.json")
     args = ap.parse_args(argv)
 
-    store = MemoryStore(args.db)
+    # #1328: read-only unless `--backfill` was asked for. `--backfill`
+    # is the one path here that legitimately writes, so it is also the
+    # only one that gets a write handle; every other invocation reads a
+    # store it must not change, and the shipped comment above points
+    # `--db` at the live file.
+    store = MemoryStore(args.db, read_only=not args.backfill)
     if args.backfill:
         report = backfill_temporal_spine(store)
         print(f"[{HARNESS_NAME}] backfill: {report}", file=sys.stderr)

src/aelfrice/store.py

@@ -1283,7 +1283,36 @@ def hash_query(query: str) -> str:
 class MemoryStore:
     """SQLite store. Pass `:memory:` for tests, a path otherwise."""
 
-    def __init__(self, path: str, *, project_context_default: str = "") -> None:
+    def __init__(
+        self,
+        path: str,
+        *,
+        project_context_default: str = "",
+        read_only: bool = False,
+    ) -> None:
+        """Open the store. `read_only=True` opens a diagnostic-safe handle.
+
+        #1328. A bare open is a **write**: it runs the DDL battery, any
+        pending migrations, the `schema_meta` seed, `_resolve_local_scope_id`
+        (which generates and persists an id on a store that has none), and —
+        since #1314 — `sweep_expired_locks`, which flips a user's expired
+        locks to unlocked. Two shipped benchmarks pointed their `--store`
+        default at the live `.git/aelfrice/memory.db` and therefore mutated
+        the corpus they existed to measure; the sweep was observed changing
+        `lock_level` on a real belief with no analysis code having run.
+
+        `read_only=True` opens the file `mode=ro` and skips the entire
+        open-time write window. SQLite then refuses writes at the engine
+        level, so the guarantee does not rest on the caller's discipline —
+        which is the point, since the previous guarantee was a sentence in a
+        docstring and it did not hold.
+
+        The schema is taken as found: no migration runs, so a store written
+        by an older binary is read at whatever shape it has. That is correct
+        for a diagnostic (it should observe the store, not upgrade it) and
+        wrong for anything that needs the current schema, which is why this
+        is opt-in rather than the default.
+        """
         # #970: repo identity stamped on new project-scope, non-user-locked
         # beliefs whose project_context is ''. Empty (the default) disables
         # stamping and the backfill — direct callers that open a store
@@ -1308,18 +1337,29 @@ def __init__(self, path: str, *, project_context_default: str = "") -> None:
         # #1176: (store_generation, active_belief_count) memo for the
         # fan-effect lane. `None` = not yet computed.
         self._active_count_memo: tuple[int, int] | None = None
-        self._conn: sqlite3.Connection = sqlite3.connect(path)
+        self._read_only: bool = read_only
+        if read_only and path != ":memory:":
+            # `mode=ro` requires the file to exist; a missing path raises
+            # here rather than silently creating an empty store, which is
+            # the right failure for a diagnostic pointed at the wrong file.
+            self._conn: sqlite3.Connection = sqlite3.connect(
+                f"file:{path}?mode=ro", uri=True
+            )
+        else:
+            self._conn = sqlite3.connect(path)
         self._conn.row_factory = sqlite3.Row
         # WAL only meaningful on-disk; harmless on :memory:.
+        # The documented WAL pairing: fsync on checkpoint, not on every
+        # commit. Durability window is the WAL — an app crash loses
+        # nothing; an OS crash can lose the tail of the WAL, which for a
+        # memory store is re-derivable (ingest_log is append-only and
+        # re-ingest is idempotent). Measured ~2x cheaper per commit than
+        # the FULL default (#1135). Both are writes, so a read-only
+        # handle skips them rather than relying on the except below.
         try:
-            self._conn.execute("PRAGMA journal_mode=WAL")
-            # The documented WAL pairing: fsync on checkpoint, not on
-            # every commit. Durability window is the WAL — an app crash
-            # loses nothing; an OS crash can lose the tail of the WAL,
-            # which for a memory store is re-derivable (ingest_log is
-            # append-only and re-ingest is idempotent). Measured ~2x
-            # cheaper per commit than the FULL default (#1135).
-            self._conn.execute("PRAGMA synchronous=NORMAL")
+            if not read_only:
+                self._conn.execute("PRAGMA journal_mode=WAL")
+                self._conn.execute("PRAGMA synchronous=NORMAL")
         except sqlite3.DatabaseError:
             pass
         # Block up to 5s waiting for a write lock instead of failing
@@ -1337,9 +1377,16 @@ def __init__(self, path: str, *, project_context_default: str = "") -> None:
         # prepare and step. The window is idempotent by construction
         # (IF NOT EXISTS / OR IGNORE / marker-gated), so re-running it
         # is safe. See `_retry_on_schema_change`.
-        self._local_scope_id: str = _retry_on_schema_change(
-            self._apply_open_schema
-        )
+        if read_only:
+            # #1328: no DDL, no migrations, no seed, no sweep. The scope id
+            # is read if present and left absent otherwise — generating one
+            # is a write, and a diagnostic has no business minting the
+            # federation identity of the store it is inspecting.
+            self._local_scope_id = self._read_only_scope_id()
+        else:
+            self._local_scope_id: str = _retry_on_schema_change(
+                self._apply_open_schema
+            )
         # #1161: every one-shot below runs through `_run_guarded_migration`
         # so a raising pass cannot make the store unopenable. Ordering is
         # unchanged and the guard does not alter the success path — see
@@ -1669,8 +1716,18 @@ def _run_guarded_migration(self, pass_: Callable[[], object]) -> bool:
         methods themselves still raise, so a test or repair tool that
         calls one directly sees the exception unchanged.
 
-        Returns True if the pass completed, False if it raised.
-        """
+        Returns True if the pass completed *or was skipped*, False only
+        if it raised. The skip case is the #1328 read-only handle below:
+        there is no third return value, and True is the fail-safe of the
+        two, because False is consumed as "record a failure marker" —
+        which is itself a write.
+        """
+        if self._read_only:
+            # #1328: every pass here writes, and a read-only handle exists
+            # precisely so a diagnostic cannot. Gated once here rather than
+            # at each of the eleven call sites, so a twelfth added later is
+            # covered without anyone remembering to cover it.
+            return True
         name = getattr(pass_, "__name__", repr(pass_))
         key = f"{SCHEMA_META_MIGRATION_FAILED_PREFIX}{name}"
         try:
@@ -1748,6 +1805,24 @@ def local_scope_id(self) -> str:
         """
         return self._local_scope_id
 
+    def _read_only_scope_id(self) -> str:
+        """The persisted scope id, or `""` if the store has none (#1328).
+
+        `_resolve_local_scope_id` mints and persists an id when the key is
+        absent, which is a write. On a read-only handle the honest answer
+        for a store that has never had one is "none" — the value is only
+        consumed by the write paths (`_bump_belief_version` and friends),
+        and those cannot run here anyway.
+
+        Tolerates a store whose `schema_meta` table does not exist yet,
+        because a read-only open runs no DDL and must not assume the
+        current schema.
+        """
+        try:
+            return self.get_schema_meta(SCHEMA_META_LOCAL_SCOPE_ID) or ""
+        except sqlite3.DatabaseError:
+            return ""
+
     def _resolve_local_scope_id(self) -> str:
         """Read the persisted scope id, generating one on first open."""
         existing = self.get_schema_meta(SCHEMA_META_LOCAL_SCOPE_ID)