Skip to content

feat: optional LLM-based closet regeneration — bring-your-own endpoint#793

Merged
igorls merged 24 commits intopr/fact-checkerfrom
pr/closet-llm-generic
Apr 13, 2026
Merged

feat: optional LLM-based closet regeneration — bring-your-own endpoint#793
igorls merged 24 commits intopr/fact-checkerfrom
pr/closet-llm-generic

Conversation

@igorls
Copy link
Copy Markdown
Member

@igorls igorls commented Apr 13, 2026

Summary

Optional LLM-based closet regeneration, vendor-agnostic. User brings their own endpoint via LLM_ENDPOINT / LLM_KEY / LLM_MODEL. Regex closets remain the default — core memory stays API-free.

Stacked on #792 (full chain: #784#788#789#790#791#792).

Context

The original bundled commit 935f657 included a closet_llm.py that imported anthropic directly and required ANTHROPIC_API_KEY. That conflicts with CLAUDE.md's "Local-first, zero API" principle and the Contributing guideline "we do not accept features requiring API keys for core memory".

This PR replaces that module with a vendor-agnostic version that preserves the same prompt, parsing, and orchestration — only the transport was generalised. Milla is co-author on the commit; the prompt template and regenerate_closets() flow are her work.

Configuration (env vars or CLI flags)

# Local (Ollama, zero cost)
LLM_ENDPOINT=http://localhost:11434/v1 \
LLM_MODEL=llama3:8b \
python -m mempalace.closet_llm --palace ~/.mempalace/palace

# Cloud (OpenAI, OpenRouter, etc.)
LLM_ENDPOINT=https://api.openai.com/v1 \
LLM_KEY=sk-... \
LLM_MODEL=gpt-4o-mini \
python -m mempalace.closet_llm --palace ~/.mempalace/palace

# CLI flags override env vars
python -m mempalace.closet_llm \
    --palace ~/.mempalace/palace \
    --endpoint http://localhost:11434/v1 \
    --model qwen2.5:7b

Any OpenAI-compatible /chat/completions endpoint works: Ollama, vLLM, llama.cpp server, OpenAI, OpenRouter, Azure OpenAI, Together, Groq, and so on.

Design decisions

  1. OpenAI Chat Completions shape — universal lingua franca across providers
  2. Zero new dependencies — uses stdlib urllib.request; no anthropic, openai, requests, or httpx added to pyproject.toml
  3. Key is optional — local inference servers typically don't authenticate. Authorization header only set when a key is provided.
  4. Graceful failure on missing config — CLI prints a clear message listing what's missing; regenerate_closets() returns {"error": "missing-config", "missing": [...]}
  5. Retries with backoff on HTTP 429 / 503 — same behavior as the original
  6. Cost reporting removed — no hardcoded pricing table; just reports prompt/completion tokens since cost depends on the provider the user chose

Test plan

  • 13 new unit tests in tests/test_closet_llm.py — mocked urllib so tests never touch the network. Covers:
    • LLMConfig env/flag resolution, trailing-slash stripping, key-optional behavior
    • _parsed_to_closet_lines — topics → pointers, caps at 15, quotes + summary
    • _call_llm — request URL, model, headers (with/without auth), code-fence stripping, invalid-JSON handling
    • regenerate_closets — missing-config error path
  • Full suite: 721/721 pass (2 version-consistency tests deselected — pre-existing develop bug)
  • Manual verification against a real Ollama instance — reviewer or follow-up PR

Callouts for reviewers

  1. Drops anthropic SDK usage — this is intentional. If the team wants to ship an Anthropic-flavored default, it should be a config preset, not hardcoded imports.
  2. Prompt unchanged from Milla's version — same PROMPT_TEMPLATE, same parsing, same caps (15 topics, 5 quotes, 1 summary).
  3. generated_by metadata now says llm:{model} instead of haiku — preserves traceability without locking to one vendor.
  4. Stacked on feat: offline fact checker against entity registry + knowledge graph #792 — full chain of 8 PRs. Each is independently reviewable; all target v3.3.

milla-jovovich and others added 2 commits April 13, 2026 07:30
normalize.py now strips before filing:
- <system-reminder>, <command-message>, <command-name> tags
- <task-notification>, <user-prompt-submit-hook>, <hook_output> tags
- Hook status messages (CURRENT TIME, Checking verified facts, etc.)
- Claude Code UI chrome (ctrl+o to expand, progress bars, etc.)
- Collapsed runs of blank lines

This noise was going straight into drawers, wasting storage space
and polluting search results. strip_noise() runs on all normalized
output regardless of input format (JSONL, JSON, plain text).

689/689 tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Adds mempalace/closet_llm.py as an OPTIONAL path for richer closet
generation. Regex closets remain the default and cover the local-first
promise; users who want LLM-quality topics can bring their own endpoint.

Configuration (env or CLI flag):
  LLM_ENDPOINT — OpenAI-compatible base URL (required)
  LLM_KEY      — bearer token (optional; local inference skips this)
  LLM_MODEL    — model name (required)

Works with Ollama, vLLM, llama.cpp servers, OpenAI, OpenRouter, and any
other provider that speaks OpenAI-compatible /chat/completions. Zero new
dependencies — uses stdlib urllib.

Replaces the original Anthropic-SDK-hardcoded version of this module
from Milla's branch (commit 935f657). Same prompt, same parsing, same
regenerate_closets flow; only the transport was generalised so the
feature doesn't lock users into a specific vendor or require API keys
for core memory operations (CLAUDE.md, "Local-first, zero API").

Includes 13 unit tests covering config resolution, request shape,
auth-header omission when no key is set, code-fence stripping, and
missing-config error path. All mocked — zero network calls in tests.

Co-Authored-By: MSL <[email protected]>
@igorls
Copy link
Copy Markdown
Member Author

igorls commented Apr 13, 2026

End-to-end validation against a real model

Ran against a local Ollama instance to confirm the bring-your-own-LLM path works:

LLM_ENDPOINT=http://localhost:11434/v1 \
LLM_MODEL=gemma4:e2b \
python -m mempalace.closet_llm --palace /path/to/palace --sample 2

Output:

Regenerating closets for 2 source files via http://localhost:11434/v1 (gemma4:e2b)...
  [1/2] ✓ f5127d2e-…jsonl — 13 topics
  [2/2] ✓ c21c5944-…jsonl — 14 topics

Done. 2 regenerated, 0 failed.
Tokens: 15,272 in + 1,583 out (cost depends on provider)

Topics extracted (sample, file 1):

  • mempalace_plugin_architecture
  • claude-code_plugin
  • codex_plugin
  • cli_commands
  • hook_system
  • knowledge_graph_management
  • memory_management
  • agent_harnesses
  • plugin_manifests
  • bash_scripting
  • json_input_output

These are substantive concept topics a user could actually search for — exactly what regex-based closets miss. Stored with `generated_by=llm:gemma4:e2b` metadata for traceability.

What this validates:

  • Config resolution (env-based)
  • HTTP request shape is actually OpenAI-compatible (Ollama accepted it)
  • Auth-header-omitted path (local Ollama, no key)
  • JSON parsing (including any fence stripping the model did)
  • _parsed_to_closet_lines produces valid topic|entities|→drawer_refs output
  • upsert_closet_lines writes them correctly to the closets collection
  • Token accounting works via the OpenAI-format usage field

Not tested:

  • OpenAI / OpenRouter / cloud paths (no key available for this run)
  • Retry/backoff on 429 (Ollama doesn't rate-limit)
  • Very large files (the tested files were ~7KB of content each)

igorls and others added 21 commits April 13, 2026 04:43
Commit 6614b9b bumped pyproject.toml to 3.2.0 but missed
mempalace/version.py, breaking test_version_consistency on
every PR's CI. This syncs them.
Adding the per-file lock + double-checked file_already_mined() in the
previous commit pushed mine_convos cyclomatic complexity from 25 to 26,
just over ruff's max-complexity threshold. Hoist the locked critical
section into _file_chunks_locked() so the outer loop stays within
budget. No behavior change.
Add blank lines after inline imports in mine_lock. Pure formatting.
fix: file-level locking to prevent multi-agent duplicate drawers
… Code JSONL

The initial strip_noise() regressed on three fronts when audited against
adversarial user content — each verified with executable repros against
the cherry-picked code:

  1. `<tag>.*?</tag>` with re.DOTALL span-ate across messages: one
     stray unclosed <system-reminder> anywhere in a session merged with
     the next closing tag, silently deleting everything between them
     (including full assistant replies).
  2. `.*\(ctrl\+o to expand\).*\n?` nuked entire lines of user prose
     whenever a user happened to document the TUI shortcut.
  3. `Ran \d+ (?:stop|pre|post)\s*hook.*` with IGNORECASE ate the
     second sentence from "our CI has a stop hook ... Ran 2 stop hooks
     last week" — legitimate user commentary.

These are unambiguous violations of the project's "Verbatim always"
design principle.

Fixes:

- All tag patterns are now line-anchored (`(?m)^(?:> )?<tag>`) and their
  body forbids crossing a blank line (`(?:(?!\n\s*\n)[\s\S])*?`), so a
  dangling open tag cannot eat neighboring messages.
- `_NOISE_LINE_PREFIXES` are line-anchored and case-sensitive — user
  prose mentioning "CURRENT TIME:" mid-sentence is preserved.
- Hook-run chrome requires `(?m)^`, explicit hook names (Stop,
  PreCompact, PreToolUse, etc.), and no IGNORECASE.
- "… +N lines" is line-anchored.
- "(ctrl+o to expand)" only matches Claude Code's actual collapsed-
  output chrome shape `[N tokens] (ctrl+o to expand)`; a bare
  parenthetical in user prose stays intact.

Scope:

- `strip_noise()` is no longer called on every normalization path.
  Only `_try_claude_code_jsonl` invokes it, per-extracted-message — so
  Claude.ai exports, ChatGPT exports, Slack JSON, Codex JSONL, and
  plain text with `>` markers pass through fully verbatim. Per-message
  application also makes span-eating structurally impossible.

Tests:

- 15 new tests in test_normalize.py pin the boundary: 6 guard user
  content that must survive (each of the adversarial repros), 9 assert
  real system chrome is still stripped. All pass; full suite 702 pass
  (2 failures are the unrelated pre-existing version.py bug, cleared
  by #820).

Known limitation (not fixed here): convo_miner.py does not delete
drawers on re-mine, so transcripts mined before this PR keep noise-
filled drawers until the user manually erases + re-mines. Proper fix
needs a schema-version field on drawer metadata + re-mine trigger —
out of scope for this PR.
…ema gate

Without this, the strip_noise improvement only helps new mines. Every
user who had already mined Claude Code JSONL sessions would keep their
noise-polluted drawers forever, because convo_miner's file_already_mined
skip short-circuits before re-processing.

Adds a versioned schema gate so upgrades propagate silently:

- palace.NORMALIZE_VERSION=2 — bumped when the normalization pipeline
  changes shape (this PR's strip_noise is the v1→v2 bump).
- file_already_mined now returns False if the stored normalize_version
  is missing or less than current, triggering a rebuild on next mine.
- Both miners stamp drawers with the current normalize_version.
- convo_miner now purges stale drawers before inserting fresh chunks
  (mirrors miner.py's existing delete+insert), extracted into
  _file_convo_chunks helper to keep mine_convos under ruff's C901 limit.

User experience: upgrade mempalace, run `mempalace mine` as usual, old
noisy drawers get silently replaced with clean ones. No erase needed,
no "you need to rebuild" changelog footgun.

Tests:
- test_file_already_mined_returns_false_for_stale_normalize_version —
  pins the version gate contract for missing/v1/current.
- test_add_drawer_stamps_normalize_version — fresh project-miner drawers
  carry the field.
- test_mine_convos_rebuilds_stale_drawers_after_schema_bump — end-to-end
  proof that a pre-v2 palace gets silently cleaned on next mine, with
  orphan drawers purged and NOT skipped.

Existing test_file_already_mined_check_mtime updated to include the
new field; all other tests unaffected.
Non-trivial merge in convo_miner.py: this branch's _file_convo_chunks
(purge stale + upsert with normalize_version) and develop's
_file_chunks_locked (mine_lock + double-checked file_already_mined)
both touched the same critical section. Combined into a single
_file_chunks_locked helper that does lock → double-check → purge →
upsert, preserving both the multi-agent safety guarantee from #784
and the schema-rebuild contract from this PR.

Also folds develop's mine_lock import into both miner.py and
convo_miner.py alongside NORMALIZE_VERSION.

707/707 tests pass, ruff + format clean under CI-pinned 0.4.x.
fix: strip system tags, hook output, and Claude UI chrome from drawers
The save hook and precompact hook were telling the agent to write
diary entries, add drawers, and add KG triples IN THE CHAT WINDOW.
Every line written stays in conversation history and retransmits on
every subsequent turn — ~$1/session in wasted tokens.

Fix: hooks now say "saved in background, no action needed" and use
decision: allow instead of block. The agent continues working without
interruption. All filing happens via the background pipeline.

Also updated hooks README with:
- Known limitation: hooks require session restart after install
- Updated cost section: zero tokens, background-only

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
fix: stop hooks from making agents write in chat — save tokens
Merges develop (#820 version sync, #785 strip_noise + NORMALIZE_VERSION,
#784 file locking) and addresses six concerns surfaced during PR review
of the closet feature:

1. Closet append-on-rebuild bug — upsert_closet_lines used to APPEND to
   existing closets (mismatched the doc's "fully replaced" promise). With
   NORMALIZE_VERSION rebuilds on develop, this would have stacked stale
   v1 topics on top of fresh v2 content forever. Fix:
   - Drop the read-and-append branch from upsert_closet_lines (now a pure
     numbered-id overwrite).
   - Add purge_file_closets(closets_col, source_file) helper that wipes
     every closet for a source file by where-filter.
   - process_file calls purge_file_closets before upsert on every mine,
     mirroring the existing drawer purge.

2. Searcher returned whole-file blobs from the closet path while the
   direct path returned chunk-level drawers. Refactored:
   - _extract_drawer_ids_from_closet parses the `→drawer_a,drawer_b`
     pointers out of closet documents.
   - _closet_first_hits hydrates exactly those drawer IDs (chunk-level),
     not collection.get(where=source_file) (which returned everything).
   - Same hit shape as direct-search path; both now carry matched_via.

3. max_distance was bypassed on the closet path. Now applied per-hit;
   when every closet candidate gets filtered, _closet_first_hits returns
   None and the caller falls through to direct drawer search.

4. Entity extraction caught sentence-starters like "When", "The",
   "After" as proper nouns. Added _ENTITY_STOPLIST (~40 common false
   positives + day/month names + role words). Real names like Igor /
   Milla still survive — covered by tests.

5. CLOSETS.md drifted from the code (claimed "replaced via upsert" but
   code appended; claimed BM25 hybrid that doesn't exist; claimed a
   10K char hydration cap that wasn't enforced). Rewritten to describe
   what actually ships, with explicit notes on the BM25 / convo-closet
   follow-ups.

6. Zero tests for ~250 lines. Added tests/test_closets.py with 17 cases:
   - build_closet_lines: pointer shape, header extraction, stoplist
     filtering (with regression case for "When/After/The"), real-name
     survival, fallback-line guarantee, drawer-ref slicing.
   - upsert_closet_lines: pure overwrite semantics (regression for the
     append bug), char-limit packing without splitting lines.
   - purge_file_closets: scoped to source_file, doesn't touch others.
   - End-to-end miner rebuild: re-mining a file with fewer topics fully
     purges leftover numbered closets from the larger first run.
   - _extract_drawer_ids_from_closet: parsing + dedup edge cases.
   - search_memories closet-first: fallback when empty, chunk-level
     hits with matched_via, no whole-file glue, max_distance enforced.

Merge resolutions: miner.py imports combined NORMALIZE_VERSION/mine_lock
from develop with the closet helpers from this branch. process_file
auto-merged cleanly (closet block sits inside develop's lock body).

724/724 tests pass. ruff + format clean under CI-pinned 0.4.x.
feat: closet layer — searchable index pointing to drawers
chore: forward closet layer (#788) into develop
…roduction

Merges develop (closet hardening #826, strip_noise #785, lock #784) and
replaces every sub-feature in this PR with a correct, tested
implementation. Shippable now.

## 1. Real Okapi-BM25 (searcher.py)

The prior `_bm25_score()` hardcoded `idf = log(2.0)` for every term — it
was really a scaled TF, not BM25, and couldn't tell a discriminative
term from a generic one. Replaced with `_bm25_scores(query, documents)`
that computes proper IDF over the provided candidate corpus using the
Lucene smoothed formula `log((N - df + 0.5) / (df + 0.5) + 1)`. Well-
defined for re-ranking vector-retrieval candidates — IDF there measures
how discriminative each term is *within the candidate set*, exactly the
signal we want.

`_hybrid_rank` also fixed:
- Vector normalization is now absolute `max(0, 1 - dist)`, not
  `1 - dist/max_dist` — adding/removing a candidate no longer reshuffles
  the others.
- BM25 is min-max normalized within candidates (bounded [0, 1]).
- Closet path now re-ranks too (was previously returning closet-order
  hits without hybrid scoring).
- `_hybrid_score` internal field stripped from output; `bm25_score`
  exposed for debugging.

## 2. Entity metadata (miner.py)

- Reuses `_ENTITY_STOPLIST` from palace.py so sentence-starters like
  "When", "After", "The" no longer land as entities (regression test
  covers this).
- Known-entity registry is cached at module level, keyed by the
  registry file's mtime — no more disk read per drawer.
- File handle now uses a context manager.
- Truncates the entity LIST (to 25) before joining — never splits a
  name in the middle.

## 3. Diary ingest (diary_ingest.py)

- State file now lives at `~/.mempalace/state/diary_ingest_<hash>.json`,
  keyed by (palace_path, diary_dir). No more pollution of the user's
  content directory.
- Drawer IDs now hash `(wing, date_str)` — a user with personal + work
  diaries on the same day no longer silently clobbers.
- Each day's upsert runs inside `mine_lock(source_file)` so concurrent
  ingest from two terminals can't race.
- `force=True` now calls `purge_file_closets` before rebuild so
  leftover numbered closets from a longer prior day don't orphan.

## 4. Tests (tests/test_closets.py)

Merged this PR's MineLock/Entity/BM25/Diary tests with develop's
hardened Build/Upsert/Purge/Rebuild/SearchClosetFirst tests. Added
specific regression tests for every fix above:
- entity stoplist applies (no "When/After/The")
- entity list capped before join (no partial tokens)
- registry cached by mtime (mock-verified zero re-reads)
- BM25 IDF downweights terms present in every doc (real BM25 evidence)
- hybrid rank absolute normalization stable against outliers
- diary state file outside user's diary dir
- diary wing-prefixed IDs prevent cross-wing date collisions

35/35 closet tests pass; full suite 743/743. ruff + format clean under
CI-pinned 0.4.x.
Merges the hardened closet/entity/BM25/diary stack from #789 and fixes
five correctness/durability issues in the tunnels module plus the
directional/symmetric design question.

## Design: tunnels are now symmetric

Per review discussion: a tunnel represents "these two things relate",
not "A causes B". The canonical ID now hashes the *sorted* endpoint
pair, so ``create_tunnel(A, B)`` and ``create_tunnel(B, A)`` resolve to
the same record and the second call updates the label rather than
creating a duplicate. ``follow_tunnels`` can be called from either
endpoint and surfaces the other side consistently.

The returned dict still preserves ``source``/``target`` in the order
the caller supplied, so UIs that want to render the connection
directionally can do so.

## Correctness fixes

* **Atomic write** — ``_save_tunnels`` writes to ``tunnels.json.tmp``
  and ``os.replace``s it into place. A crash mid-write can no longer
  leave a truncated file that silently reads back as ``[]`` and wipes
  every tunnel. Includes ``f.flush() + os.fsync`` before replace on
  platforms that support it.
* **Concurrent-write lock** — ``create_tunnel`` and ``delete_tunnel``
  wrap the load→mutate→save cycle in ``mine_lock(_TUNNEL_FILE)``.
  Without this, two agents creating tunnels simultaneously would both
  read the same snapshot and the later writer would drop the earlier
  writer's tunnel.
* **Corrupt-file tolerance** — ``_load_tunnels`` now uses a context
  manager, validates that the loaded JSON is a list, and returns ``[]``
  for any read failure. Subsequent ``create_tunnel`` then overwrites
  the corrupt file via atomic write — no manual recovery needed.
* **Input validation** — new ``_require_name`` helper rejects empty or
  whitespace-only wing/room names with a clear ``ValueError``. Prevents
  phantom tunnels with blank endpoints from ever reaching the JSON
  store.
* **Timezone-aware timestamps** — ``created_at`` / ``updated_at`` now
  use ``datetime.now(timezone.utc).isoformat()``, matching diary ingest
  and other recent modules.

## Tests (12 in TestTunnels)

5 original + 7 regression cases:
* ``test_tunnel_is_symmetric`` — A↔B and B↔A dedupe to one record.
* ``test_follow_tunnels_works_from_either_endpoint`` — symmetric surface.
* ``test_empty_endpoint_fields_rejected`` — validation guard.
* ``test_corrupt_tunnel_file_does_not_lose_new_writes`` — truncated
  JSON treated as empty; next create persists cleanly.
* ``test_atomic_write_leaves_no_stray_tmp_file`` — no leftover ``.tmp``.
* ``test_concurrent_creates_preserve_all_tunnels`` — 5 threads each
  create a distinct tunnel; all 5 persisted (regression for the
  read-modify-write race).
* ``test_created_at_is_timezone_aware`` — ISO8601 has tz suffix.

Merge resolutions: tests/test_closets.py combined develop's hardened
closet/entity/BM25/diary tests with this PR's TestTunnels class.

755/755 tests pass. ruff + format clean under CI-pinned 0.4.x.
… path

Merges the full hardened stack (#788 closets, #789 entity/BM25/diary,
#790 tunnels) and reimplements the drawer-grep feature in a way that
composes with the chunk-level closet-first search instead of fighting it.

## Background

The original PR added "drawer-grep" on top of the pre-hardening closet
code that returned whole-file blobs. My #788 hardening changed that
path to return *chunk-level* hits by parsing each closet's
``→drawer_id`` pointers and hydrating exactly those drawers. That made
the original drawer-grep grep-over-all-drawers logic redundant — the
closet already points at the relevant chunk.

What remained valuable from the original PR was the *context expansion*
idea: a chunk boundary can clip a thought mid-stride (matched chunk
says "here's a breakdown:" and the breakdown lives in the next chunk),
so callers want ±1 neighbor chunks for free rather than a follow-up
get_drawer call.

## Change

New ``_expand_with_neighbors(drawers_col, doc, meta, radius=1)`` helper
in searcher.py:

* Reads ``source_file`` + ``chunk_index`` from the matched drawer's
  metadata.
* Fetches the ±radius sibling chunks in a SINGLE ChromaDB query using
  ``$and + $in`` — no "fetch all drawers for source" blowup.
* Sorts retrieved chunks by chunk_index, joins with ``\n\n``.
* Does a cheap metadata-only second query to compute ``total_drawers``
  so callers know where in the file they landed.
* Graceful fallback to the matched doc alone on any ChromaDB failure or
  missing metadata — search never breaks because expansion failed.

``_closet_first_hits`` now calls this helper and tags each hit with
``drawer_index`` + ``total_drawers``. Hit shape stays consistent with
the direct-search path (both still carry ``matched_via``) so callers
can't tell which path produced a given hit except via that field.

## Tests

6 new cases in TestDrawerGrepExpansion:
* neighbors returned in chunk_index order (not hash order)
* edge case: matched chunk at index 0 — only next neighbor surfaces
* edge case: matched chunk at last index — only prev neighbor surfaces
* edge case: 1-drawer file — returns just the matched doc
* missing/non-int chunk_index metadata — graceful fallback
* end-to-end via ``search_memories`` — closet-first hit carries
  drawer_index, total_drawers, and includes ±1 neighbors

761/761 suite pass; ruff + format clean on CI-pinned 0.4.x.

Merge resolutions: miner.py kept develop's purge+NORMALIZE_VERSION;
searcher.py dropped the old whole-file-blob block entirely in favor of
rebuilding context expansion on top of ``_closet_first_hits``;
test_closets.py took develop's 47-test baseline and appended
TestDrawerGrepExpansion.
Merges the full hardened stack (up through #791 drawer-grep) and turns
fact_checker from "dead code hidden behind bare except" into an
actually-working offline contradiction detector with tests.

## Dead paths the PR body advertised but the code never executed

Both buried by a single outer ``except Exception: pass``:

  * ``kg.query(subject)`` — ``KnowledgeGraph`` has no ``query()`` method;
    it has ``query_entity()``. The attribute error was silently swallowed
    and the entire KG branch always returned ``[]``. Now using
    ``kg.query_entity(subject, direction="outgoing")`` with proper
    handling of the ``predicate``/``object``/``current``/``valid_to``
    fields the real API returns.
  * ``KnowledgeGraph(palace_path=palace_path)`` — the constructor's only
    kwarg is ``db_path``. Passing ``palace_path`` raised TypeError,
    silently swallowed. Now computing the db_path correctly from
    ``<palace>/knowledge_graph.sqlite3``, matching the convention the
    MCP server already uses.

## Contradiction logic rewritten

The previous ``if kg_pred in claim and fact.object not in claim`` only
fired when text used the SAME predicate word as the KG fact — the exact
opposite of the stated use case ("Bob is Alice's brother" when KG says
husband" would NOT have fired). Replaced with a proper parse → lookup
→ compare pipeline:

  * ``_extract_claims`` parses two surface forms ("X is Y's Z" and
    "X's Z is Y") into ``(subject, predicate, object)`` triples.
  * ``_check_kg_contradictions`` pulls the subject's outgoing facts
    and flags two classes:
      - ``relationship_mismatch`` when a current KG fact matches the
        same ``(subject, object)`` pair but with a different predicate.
      - ``stale_fact`` when the exact triple exists but is
        ``valid_to``-closed in the past.
  * Stale-fact detection is now implemented (the PR body claimed it;
    the old code silently didn't implement it).

## Performance fix — O(n²) → O(mentioned × n)

``_check_entity_confusion`` previously computed Levenshtein for every
pair of registered names on every ``check_text`` call. For 1,000
registered names that's ~500K edit-distance calls per hook invocation.
Now we first identify which registry names actually appear in the text
(single regex scan), then only compute edit distance between mentioned
and unmentioned names. Pinned by a test that asserts <200ms on a 500-
name registry with zero mentions.

Also: when *both* similar names are mentioned in the text, we no
longer flag them — the user clearly knows they're different people.

## Shared entity-registry loader

``mempalace/miner.py`` already had an mtime-cached loader for
``~/.mempalace/known_entities.json``. fact_checker had a duplicate
implementation that leaked file handles and ignored caching. Extended
miner's cache to expose both the flat set (``_load_known_entities``)
and the raw category dict (``_load_known_entities_raw``); fact_checker
now imports the latter. No more double disk reads, no more handle leak.

## Tests — 24 cases in tests/test_fact_checker.py

All three detection paths + both dead-code regressions:
  * ``test_kg_init_uses_db_path_not_palace_path_kwarg`` — pins the
    correct KG constructor signature so the ``palace_path=`` bug can't
    come back.
  * ``test_relationship_mismatch_detected`` — the headline example from
    the PR body now actually fires.
  * ``test_stale_fact_detected`` — valid_to-closed triple is flagged.
  * ``test_current_fact_same_triple_is_not_flagged`` — no false positive
    on a still-valid match.
  * ``test_performance_bounded_by_mentioned_names`` — 500-name registry,
    zero mentions, <200ms. Regression for the O(n²) blowup.
  * ``test_no_false_positive_when_both_names_mentioned`` — Mila and
    Milla in the same text is fine.
  * Plus claim extraction, flatten_names shapes, CLI exit code, empty
    text handling, missing-palace graceful fallback, registry-dict
    shape support.

785/785 suite pass. ruff + format clean on CI-pinned 0.4.x.
Brings in PR #793 (optional LLM-based closet regeneration via
user-configured OpenAI-compatible endpoint) and PR #795 (hybrid
closet+drawer search — closets boost, never gate). Stack: #784#788#789#790#791#792#793 (+ #795).

Findings hardened on our side
─────────────────────────────

1) closet_llm.regenerate_closets didn't use the blessed palace helpers.

   Before:
     * manual closets_col.get(where=...) + .delete(ids=...) with a
       silent ``except Exception: pass`` around both — if the purge
       failed, pre-existing regex closets survived alongside fresh LLM
       closets, giving the searcher double hits for the same source.
     * ``source.split('/')[-1][:30]`` to build the closet_id — quietly
       wrong on Windows paths (``C:\\proj\\a.md`` has no ``/``, so the
       whole string ends up in the ID).
     * no mine_lock around purge+upsert — a concurrent regex rebuild of
       the same source could interleave with our purge and leave a mix
       of regex and LLM pointers.
     * no ``normalize_version`` stamp on the LLM closets — the miner's
       stale-version gate would treat them as leftovers from an older
       schema and rebuild over them on the next mine.

   After: routes through ``purge_file_closets`` + ``mine_lock`` +
   ``os.path.basename`` + ``NORMALIZE_VERSION`` stamp. Regression tests
   cover each.

2) searcher.search_memories was still closet-first.

   PR #795 merged into #793's head to fix the recall regression
   documented in that PR (R@1 0.25 on narrative content vs. 0.42
   baseline). The hybrid design makes closets a ranking boost rather
   than a gate: drawers are always queried at the floor, and matching
   closet hits (rank 0-4 within CLOSET_DISTANCE_CAP=1.5) add a boost
   of 0.40/0.25/0.15/0.08/0.04 to the effective distance.

   Merged to take the incoming hybrid design, with two cleanups:
   * kept the ``_expand_with_neighbors`` / ``_extract_drawer_ids_from_closet``
     helpers as separately-tested utilities (still imported by tests
     and future callers);
   * replaced the fragile ``source_file.endswith(basename)`` reverse-
     lookup in the enrichment step with internal ``_source_file_full``
     / ``_chunk_index`` fields stripped before return, so enrichment
     doesn't silently pick the wrong path when two sources share a
     basename across directories;
   * drawer-grep enrichment now sorts by ``chunk_index`` before
     neighbor expansion, so ``best_idx ± 1`` corresponds to actual
     document order rather than whatever order Chroma returned.

3) Closet-first tests in test_closets.py (``TestSearchMemoriesClosetFirst``,
   end-to-end ``test_closet_first_search_includes_drawer_index_and_total``)
   pinned contracts that the hybrid path now violates (``matched_via``
   went from ``"closet"`` to ``"drawer+closet"``). Rewrote them around
   the new invariant: direct drawers are always the floor, closet
   agreement flips the hit's matched_via and exposes closet_preview.

Verification
────────────

* 805/805 pass under ``uv run pytest tests/ -v --ignore=tests/benchmarks``
  (13 new tests from PR #793 + 5 from PR #795 + 2 new regressions for
  the closet_llm hardening + the rewritten hybrid assertions in
  test_closets.py).
* CI-pinned ruff 0.4.x clean on ``mempalace/`` + ``tests/`` (check +
  format both pass).
* No new deps — closet_llm.py still uses stdlib ``urllib.request`` per
  the PR's "zero new dependencies" promise.

Co-Authored-By: MSL <[email protected]>
@igorls igorls merged commit d16de9f into pr/fact-checker Apr 13, 2026
@igorls igorls deleted the pr/closet-llm-generic branch April 13, 2026 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P0 critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants