fix(store): a re-asserted retired belief revives only for a person (#1215)

CHANGELOG/v4.md

@@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Re-asserting a statement you had retired was swallowed, and `aelf lock` reported success anyway ([#1215](https://github.com/robotrocketscience/aelfrice/issues/1215)).** [#1210](https://github.com/robotrocketscience/aelfrice/issues/1210) gave `get_belief` a `valid_to` filter; `get_belief_by_content_hash` still had none, and it is the lookup every ingest path resolves through. So a re-assertion of retired content matched the **tombstone**: `insert_or_corroborate` wrote a corroboration row against the retired belief and returned `was_inserted=False`, the INSERT never ran, and nothing became visible. Reproduced end to end — after `aelf retire`, a second `aelf lock` of the same sentence printed `upgraded existing belief to lock` while the row stayed at `valid_to` set, `aelf locked` listed nothing, and search found nothing; the store held exactly one row, invisible. That is the residual case [#1164](https://github.com/robotrocketscience/aelfrice/issues/1164) did not cover: its fix correctly moved the lock upgrade onto the *resolved* id, which is why `lock_level` did land — on a row nothing can read. The lookup now excludes retired rows by default. Two callers opt back in, both because they are **UNIQUE-constraint guards rather than reads**: `content_hash` is `NOT NULL UNIQUE` ([#219](https://github.com/robotrocketscience/aelfrice/issues/219)), so a tombstone still owns its hash and an insert that cannot see it trips the constraint — verified, not assumed: reverting the opt-in in `wonder_ingest` raises `sqlite3.IntegrityError: UNIQUE constraint failed: beliefs.content_hash` when the next wonder pass revisits a GC'd phantom. That constraint is also why "insert a fresh row alongside the tombstone" was never on the table, and the fix is a policy decision rather than a filter. **Ratified policy, tiered by who is asserting:** an explicit user assertion (`aelf lock`, `aelf remember`, and their MCP twins) **revives** the belief — `valid_to` cleared, FTS row restored, back in search — at the posterior it was retired at, with a `reassert:revive` audit row so the transition is not silent in either direction. Background capture (transcript, commit, filesystem, wonder, claude-memory mirror, migration) leaves the tombstone retired and records nothing, because an agent re-observing text it already scanned must not undo the user's curation — and under the old behaviour it was doing exactly that, accruing corroboration rows on a belief the user had removed. Revival deliberately does **not** move the posterior: the re-assertion is recorded as a `belief_corroborations` row, which is where that signal belongs. Tests assert the invariant per tier, each with a negative control on live content — without it, a bug that made capture a no-op outright would satisfy both "did not revive" and "wrote nothing".
 - **`aelf setup` mutated `settings.json` ten times over with no lock, so a concurrent writer's changes were silently discarded ([#1161](https://github.com/robotrocketscience/aelfrice/issues/1161)).** `_atomic_write` is atomic *per write* but is not a compare-and-swap: it replaces the whole file from an in-memory snapshot, so whatever another process wrote between the load and the replace is gone. `aelf setup` performed roughly ten independent read-modify-write cycles on that file, `aelf unsetup` and `aelf doctor --fix` performed their own, and none of the three held a lock of any kind. The one lock that existed — `auto_install`'s `~/.aelfrice/.auto-install.lock` — serialises the auto-install merge against *itself* and says nothing about the other three, and it is scoped to aelfrice's stamp directory rather than to the resource being written. Reproduced with two threads that load the same document and write in sequence: the first writer's key is simply absent afterwards. Reachable in ordinary use, since running two sessions concurrently is a supported workflow and every CLI invocation can trigger the auto-install merge. Mutations now run inside a `settings_transaction`, which holds an exclusive advisory lock on a sibling `<name>.lock` — never on the settings file itself, whose inode `_atomic_write` replaces — reads the document once, accumulates every installer's changes in memory, and writes once at commit. The write is skipped entirely when nothing changed, so the "already installed, skip the write" path still touches nothing. The active transaction is thread-local rather than a module global, because a process can legitimately run two of these (a threaded host, the MCP server) and a shared slot would let one thread buffer its writes into another's document. Contention is bounded: interactive commands wait up to 10 s and then report and exit non-zero rather than hang or silently do nothing, while the hook-driven auto-install merge waits 2 s and skips — its version stamp stays unwritten, so the next invocation retries. **Correction to the filed report,** which asked for the ten cycles to be collapsed into a single load-once/write-once pass: collapsing alone would have made this worse. Because each cycle re-reads the file, a host write landing *between* two installers survives today — measured, by injecting a permission grant mid-install and finding it still present afterwards — whereas one pass would widen that window from ten microsecond-scale gaps to the entire duration of `aelf setup`. What makes batching safe is the content fingerprint compared at commit: a write by any process that does not take aelfrice's lock, notably the host harness that owns this file too and cannot be made to cooperate, is detected and the transaction aborts with `SettingsChangedDuringTransaction` instead of overwriting it. Every mutation aelfrice makes to this file is convergent, so re-running the command is the remedy. `doctor`'s hook prune now reads and writes through the transaction-aware `setup.read_settings`/`write_settings`, which is load-bearing rather than cosmetic: its own atomic write would change the file underneath an open transaction and trip that very fingerprint check. `exclusive_file_lock` gained an optional bounded wait for this; its default indefinite block is unchanged for the session-ring and telemetry appends it was written for. **Not covered:** the host harness still writes this file without any lock, which no change on aelfrice's side can fix — the fingerprint check turns that from silent data loss into a reported abort, which is the best available outcome.
 
 - **One store migration could make the memory store permanently unopenable ([#1161](https://github.com/robotrocketscience/aelfrice/issues/1161)).** `MemoryStore.__init__` runs ten one-shot migrations, and every one stamps its completion marker *after* doing the work — so any exception escaping the constructor guaranteed the next open would re-run the same pass and raise again. The #219 consolidation pass, which collapses duplicate `content_hash` rows onto a canonical belief, rewrote foreign keys with a bare `UPDATE edges SET src = ?`; `edges` is keyed `PRIMARY KEY (src, dst, type)`. Whenever a duplicate and its canonical row shared an edge of the same type to the same neighbour — the *expected* shape, since duplicates are the same content ingested twice and the edge builders derive edges from content — the rewrite raised `UNIQUE constraint failed: edges.src, edges.dst, edges.type`. Confirmed by reproduction against a store regressed to the pre-#219 shape: `aelf stats`, `aelf health` and `aelf doctor` all died with the identical error on every open, and since every entry point (CLI, hook, MCP) opens the store, there was no recovery path through the package at all. Both the `src` and the `dst` rewrite could hit it, as could two duplicates sharing one target — the second rewrite colliding with the first, inside the same `executemany`. The rewrites now use `UPDATE OR IGNORE` and reap whatever still references a duplicate afterwards, so the canonical row's edge wins and no orphan is left behind (`edges` carries no foreign key to `beliefs`, so nothing else would sweep it); weights are not merged, because inventing an arithmetic there would be a silent semantic change. A `NOT IN (canon, dupe)` guard keeps an intra-group edge — an assertion of a relationship between two rows that turned out to be one belief — from being rewritten into a `(canon, canon)` self-loop, while leaving any self-loop the store already had untouched. `belief_corroborations` gets the same treatment for the partial UNIQUE index #1020 adds to it, and `edge_versions` now follows the edges it mirrors instead of stranding federation version vectors on edges the migration moved or deleted. Two further routes to the same unopenable state are closed. The `IN (...)` predicates bound one parameter per affected row, so a store with more duplicates than `SQLITE_LIMIT_VARIABLE_NUMBER` — 32766 on current builds, 999 on anything older than SQLite 3.32, and set by whichever library the interpreter happens to bundle — raised `too many SQL variables` instead; they now batch through a fixed chunk small enough to clear the older limit even where the cleanup delete binds each chunk twice. And no one-shot migration is fatal any more: each runs through a guard that records the failure under `schema_meta` as `migration_failed:<name>`, logs it at ERROR, leaves the completion marker unset so a fixed build retries the pass, and lets the open succeed. The trade is deliberate — an unopenable store is total loss of the corpus from the user's point of view, while a store that opens with one migration incomplete keeps its pre-migration shape, serves every read and write, and repairs itself on a later version. The migration methods still raise when called directly, so a repair tool or a test that drives one on purpose sees the exception unchanged. `_maybe_apply_content_hash_unique` now *checks* its no-duplicates precondition rather than assuming it, since the dedup pass it depends on is allowed to fail; without that check one incomplete migration cascaded into a second UNIQUE violation on the same open. `aelf doctor` reports any incomplete migration with its underlying error — the only signal the operator gets now that the store no longer refuses to open — reading the marker over a read-only connection rather than by constructing a `MemoryStore`, because a diagnostic must not retry every pending migration or take the write lock on the store it is inspecting.

src/aelfrice/models.py

@@ -285,6 +285,25 @@ def retention_class_for_source(source_kind: str) -> str:
     CORROBORATION_SOURCE_CLAUDE_MEMORY,
 })
 
+# #1215: the corroboration sources that are a *person* asserting a statement,
+# as opposed to background capture re-observing text. Only these revive a
+# retired belief when the same content is asserted again — re-typing a
+# sentence is a deliberate act, whereas a transcript or commit scan finding
+# it again must not silently undo the user's curation. Consulted by
+# `MemoryStore.insert_or_corroborate`; every other source in
+# CORROBORATION_SOURCE_TYPES leaves the tombstone retired.
+CORROBORATION_SOURCES_USER_EXPLICIT: Final[frozenset[str]] = frozenset({
+    CORROBORATION_SOURCE_CLI_REMEMBER,
+    CORROBORATION_SOURCE_MCP_REMEMBER,
+})
+
+# #1215: feedback_history `source` for the revival above. Audit only — it
+# records *why* a retired belief came back so the transition is not silent,
+# and carries valence 0.0 because the posterior is deliberately preserved at
+# the value the belief was retired at. The re-assertion itself is recorded as
+# a belief_corroborations row, which is where that signal belongs.
+FEEDBACK_SOURCE_REASSERT_REVIVE: Final[str] = "reassert:revive"
+
 # v2.0 #205 ingest_log source_kind enum. Wire-format strings; do not
 # rename without a migration. Spec: docs/design/write-log-as-truth.md.
 INGEST_SOURCE_FILESYSTEM: Final[str] = "filesystem"

src/aelfrice/store.py

@@ -36,10 +36,12 @@
     CORROBORATION_SOURCE_CONSOLIDATION_MIGRATION,
     CORROBORATION_SOURCE_TYPES,
     CORROBORATION_SOURCE_WONDER_INGEST,
+    CORROBORATION_SOURCES_USER_EXPLICIT,
     EDGE_RELATES_TO,
     EDGE_SUPERSEDES,
     EDGE_VALENCE,
     EXPOSURE_ONLY_FEEDBACK_SOURCES,
+    FEEDBACK_SOURCE_REASSERT_REVIVE,
     ORIGIN_SPECULATIVE,
     INGEST_SOURCE_KINDS,
     INGEST_SOURCE_LEGACY_UNKNOWN,
@@ -2617,22 +2619,33 @@ def insert_belief(self, b: Belief) -> None:
         self._bump_belief_version(b.id)
         self._commit_mutation()
 
-    def get_belief_by_content_hash(self, content_hash: str) -> Belief | None:
+    def get_belief_by_content_hash(
+        self, content_hash: str, *, include_retired: bool = False
+    ) -> Belief | None:
         """Look up a belief by its content_hash. Returns None if not found.
 
         Used by ingest paths to detect re-ingest of identical content
         across different (source, text) pairs — e.g. the same sentence
         ingested once via transcript-ingest and once via commit-ingest.
         When found, the caller records a belief_corroborations row
         instead of silently dropping the duplicate.
+
+        Retired rows are excluded by default, matching :meth:`get_belief`
+        (#1210). ``include_retired=True`` is for the two callers that use
+        this as a *UNIQUE-constraint guard* rather than to read content:
+        `content_hash` is `NOT NULL UNIQUE` (#219), so a tombstone still
+        owns its hash and an insert that cannot see it trips the
+        constraint. Those callers must then decide what a re-assertion of
+        retired content means — see :meth:`insert_or_corroborate` (#1215).
         """
+        lifecycle = "" if include_retired else "AND b.valid_to IS NULL"
         cur = self._conn.execute(
-            """
+            f"""
             SELECT b.*,
                    (SELECT COUNT(*) FROM belief_corroborations bc
                     WHERE bc.belief_id = b.id) AS corroboration_count
             FROM beliefs b
-            WHERE b.content_hash = ?
+            WHERE b.content_hash = ? {lifecycle}
             LIMIT 1
             """,
             (content_hash,),
@@ -3669,6 +3682,33 @@ def insert_or_corroborate(
         `source_type` must be in CORROBORATION_SOURCE_TYPES; ValueError
         is raised immediately on an unknown value so the caller's test
         suite catches misconfigured mappings early.
+
+        **Re-assertion of retired content (#1215).** The content-hash
+        lookup opts into retired rows because `content_hash` is UNIQUE
+        (#219) — a tombstone still owns its hash, so an insert that could
+        not see it would trip the constraint. That makes what happens
+        next a policy question, and before #1215 the answer was the worst
+        one available: the re-assertion was swallowed, a corroboration
+        row was written against the tombstone, and nothing became
+        visible. `aelf lock` on a retired statement printed success while
+        `aelf locked` stayed empty.
+
+        The ratified policy is tiered by who is asserting:
+
+        - **A person** (`CORROBORATION_SOURCES_USER_EXPLICIT` — `aelf
+          lock`, `aelf remember`, and their MCP twins) **revives** the
+          belief. Re-typing a sentence is a deliberate act, and it comes
+          back at the posterior it was retired at, with an audit row
+          naming the revival.
+        - **Background capture** (transcript, commit, filesystem, wonder,
+          claude-memory mirror, migration) leaves the tombstone retired
+          and records nothing. An agent re-observing text it already saw
+          must not undo the user's curation.
+
+        The skip path still returns ``(existing.id, False)``: the content
+        genuinely did resolve to that row, and the ingest-log stamp
+        should say so. The caller learns nothing was inserted, which is
+        true either way.
         """
         # Validate source_type up-front so the error surfaces at the
         # call site, not inside record_corroboration after the lookup.
@@ -3677,7 +3717,22 @@ def insert_or_corroborate(
                 f"Unknown source_type {source_type!r}. "
                 f"Must be one of {sorted(CORROBORATION_SOURCE_TYPES)}"
             )
-        existing = self.get_belief_by_content_hash(b.content_hash)
+        existing = self.get_belief_by_content_hash(
+            b.content_hash, include_retired=True
+        )
+        if existing is not None and existing.valid_to is not None:
+            # #1215: re-assertion of retired content. Only a person
+            # revives it; background capture leaves it retired and
+            # writes nothing, so no tombstone accrues evidence.
+            if source_type not in CORROBORATION_SOURCES_USER_EXPLICIT:
+                return (existing.id, False)
+            self.restore_belief(existing.id)
+            self.insert_feedback_event(
+                belief_id=existing.id,
+                valence=0.0,
+                source=FEEDBACK_SOURCE_REASSERT_REVIVE,
+                created_at=datetime.now(timezone.utc).isoformat(),
+            )
         if existing is not None:
             self.record_corroboration(
                 existing.id,

src/aelfrice/wonder/lifecycle.py

@@ -155,7 +155,13 @@ def wonder_ingest(
         key = _constituent_key(
             phantom.constituent_belief_ids, phantom.generator
         )
-        existing = store.get_belief_by_content_hash(key)
+        # include_retired (#1215): this is a UNIQUE-constraint guard, not a
+        # read. `content_hash` is `NOT NULL UNIQUE` (#219), so a GC'd phantom
+        # still owns its constituent key — taking the default would re-insert
+        # and trip the constraint. Skipping is also the right answer on its
+        # own terms: a phantom the lifecycle already retired should not be
+        # regenerated by the next wonder pass.
+        existing = store.get_belief_by_content_hash(key, include_retired=True)
         if existing is not None:
             skipped += 1
             continue