Skip to content

chore: sync upstream/develop into main (2026-05-07) - #15

Merged
jphein merged 119 commits into
mainfrom
chore/sync-develop-2026-05-07
May 8, 2026
Merged

jphein merged 119 commits into
mainfrom
chore/sync-develop-2026-05-07

Conversation

@jphein

@jphein jphein commented May 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Merges 113 commits from upstream/develop (1888b67 → ea36a00).
  • 53 files changed, +6,077/-404.
  • 8 conflicts resolved by hand; 1 follow-up fix on _count_in_scope so upstream's mock-based searcher tests pass against our BM25-fallback path.
  • Tests: 1,733 passed, 1 skipped, 106 deselected (was 1,591 pre-merge).

Notable upstream items now in tree

Hash What
f0d2360 Our PR MemPalace#1377 (retry-once on _get_collection) — JP credited as co-author.
5134a63 SQLite integrity preflight before chromadb open (fatkobra/fix/1362).
5488e7b Windows mine hardening against ONNX bad_alloc + silent partial exits (fix/1296).
a7c4ed2 New --mode from-sqlite repair path (PR MemPalace#1310, potterdigital).
2ff6283 Diary content-hash detection for same-size edits (fix/925).
bddba59 4 new auto-save tools under tools/ (PR MemPalace#1391).

Conflict resolutions

  • .gitignore — keep both sides (ours dashboards-bensig-*/ + upstream's .envrc).
  • README.md — keep fork README per feedback_fork_readme_handling. Main carries our README; PR branches carry upstream's.
  • mempalace/palace.py — take upstream's logger = logging.getLogger("mempalace_mcp") to reduce fork drift.
  • mempalace/cli.py — extend our existing --mode choices to add from-sqlite; take upstream's new --source and --archive-existing flags; drop upstream's duplicate --mode block (HEAD already had one from row 23).
  • mempalace/hooks_cli.py — keep our richer precompact docstring (closes Copilot fix(hooks): drop checkpoint write path (verbatim-only, part 1) #6) AND adopt upstream's new _palace_root_exists() short-circuit guard.
  • mempalace/searcher.py (3 blocks) — keep delegate-to-search_memories CLI path (row 12) and drawer_id zip-with-id-pad (row 25); add upstream's defensive doc = doc or "".
  • tests/test_hooks_cli.py — keep our mock_ingest.called assertion + adopt upstream's 5 new "absent palace root → short-circuit, never mkdir" tests.
  • tests/test_searcher.py — keep both fork's BM25-fallback test (row 12) and upstream's effective_distance clamp test (row 29 territory).

Follow-up fix

mempalace/searcher.py::_count_in_scope was annotated Optional[int] but returned whatever drawers_col.count() returned — leaking MagicMock through to the BM25-fallback comparison and tripping upstream's new mock-based tests. Coerced at the boundary: int(raw) if isinstance(raw, (int, float)) else None. Production untouched (real chromadb always returns int).

Test plan

  • python -m pytest tests/ -q → 1,733 passed
  • python3 -c "import ast; ..." AST-parses all 6 conflicted Python files
  • bash scripts/check-docs.sh — checks 1-4 pass; check 5 has 4 pre-existing failures unrelated to this sync (commits 42817d7, 7ba28dc, 8252025, f9f5cc4 — all in main before this PR; tracked separately)
  • CI on jphein/mempalace
  • Copilot review on the merge resolutions

eldar702 and others added 30 commits April 19, 2026 11:13
Chroma 1.5.x can return ``None`` inside the ``metadatas`` / ``documents``
lists of a query/get result for partially-flushed rows. The codebase
already has a systemic None-guard pattern (merged MemPalace#999, MemPalace#1013, MemPalace#1019)
but three call sites were still unguarded:

* ``mcp_server.tool_check_duplicate`` (``mcp_server.py:487-488``) —
  ``meta = results["metadatas"][0][i]`` followed by ``meta.get(...)``
  raises ``AttributeError: 'NoneType' object has no attribute 'get'``.
  The broad ``except Exception`` wrapper (line 504) swallows it and
  returns an uninformative ``"Duplicate check failed"``.

* ``layers.Layer1.generate`` (``layers.py:126``) — iterates
  ``zip(docs, metas)`` and calls ``meta.get(key)`` in the importance
  loop. A single None metadata blows up the entire wake-up render.

* ``layers.Layer2.retrieve`` (``layers.py:224``) — same pattern, same
  crash path for the on-demand render.

Apply the same ``meta = meta or {}`` / ``doc = doc or ""`` idiom used
by the merged guards in the search path. Three-line additions, no
behaviour change on well-formed results.

Tests added:

* ``test_check_duplicate_handles_none_metadata`` — mocks the collection
  query to return ``None`` for one metadata and document, asserts the
  call does not crash and the sentinel-rendered entry has wing/room "?"
  and empty content.
* ``test_layer1_handles_none_metadata`` / ``_handles_none_document``
* ``test_layer2_handles_none_metadata``

Relationship to other open PRs:

* **MemPalace#1019** guarded ``searcher.py`` loops. This PR extends the same
  guard to the three call sites MemPalace#1019 did not touch.
* **MemPalace#979** fixed ``tool_check_duplicate`` negative similarity but left
  the None-metadata path unguarded.
* Does not overlap **MemPalace#1013** (``Layer3.search_raw``) or **MemPalace#999**.
On Windows, Python defaults sys.stdin/sys.stdout to the system codepage
(e.g. cp1251 on Russian locales, cp1252 on Western European), while MCP
JSON-RPC is always UTF-8. Non-ASCII payloads (Cyrillic, CJK, accented
European) get mis-decoded before reaching handlers, causing json.loads
to fail or tool handlers to receive garbled strings. Both surface to
the client as a generic MCP error -32000.

Reproduction:
  1. On Windows with a non-Latin locale, call mempalace_add_drawer or
     mempalace_kg_add with Cyrillic/CJK in content or KG object.
  2. Server returns: MCP error -32000: Internal tool error.
  3. Calling the handler directly from Python works fine -- the bug is
     purely in the stdio transport.

Fix:
  Reconfigure stdin/stdout to UTF-8 at the start of main(), after
  _restore_stdout(). Uses errors="replace" defensively so a lone bad
  byte cannot take down the server. Guarded by hasattr(reconfigure)
  for exotic stream replacements.

This matches the behaviour of PYTHONUTF8=1 / python -X utf8 without
requiring users to set an env var.
The list_drawers response only included count (current page size) with
no total field, making it impossible for callers to know when pagination
is exhausted. A page returning count == limit is ambiguous — it could
be the last exact-fit page or there could be more results.

Add a total field that reports the full number of matching drawers.
For unfiltered requests this uses col.count(); for filtered requests
(wing/room) it uses a lightweight col.get(include=[]) to count
matching IDs without fetching documents.
A triple with valid_to < valid_from satisfies neither of the temporal
filter clauses in query_entity():

    valid_from <= as_of AND valid_to >= as_of

so the triple is invisible to every query — silently corrupt. Reject
at write time with a clear error instead of letting bad data pile up
in the SQLite store.

The guard only fires when both bounds are present; open intervals
(only valid_from or only valid_to) are still accepted, and same-day
intervals (valid_from == valid_to, point-in-time facts) are explicitly
allowed.
MemPalace#976 protects `mempalace mine`, but MCP/direct backend writers still call
ChromaCollection.add/upsert/update/delete without the palace lock. This
moves the lock boundary to the Chroma backend seam so all Chroma writes
share the same palace-level serialization, with a re-entrant guard for
miner paths that already hold the lock.

mine_palace_lock(palace_path) gains a per-thread re-entrant guard
(threading.local + pid-tag against fork inheritance) so
ChromaCollection write methods can take the lock without
self-deadlocking when called from inside miner.mine()'s outer hold.

ChromaCollection.__init__ accepts an optional palace_path; when set,
add/upsert/update/delete wrap their underlying chromadb call with
mine_palace_lock(palace_path). palace_path=None preserves the legacy
no-lock behaviour for direct callers and tests. ChromaBackend's
get_collection/create_collection pass palace_path through;
mcp_server._get_collection forwards _config.palace_path so all MCP
write tools inherit the wrapping.

Tests: 5 new in tests/test_chroma_collection_lock.py covering opt-in,
writer-blocks-during-mine, re-entrant-inside-mine, two-process
serialization, and a source-level read-path-not-locked pin. Plus 1 new
+ 1 rewritten in tests/test_palace_locks.py for the re-entrant
semantics. 52 passed in 1.01s including the existing test_backends.py
regression suite.

Refs MemPalace#1161.
Mirror the pagination pattern PR MemPalace#851 landed in miner.py:status().
A single drawers_col.get(limit=total, ...) on palaces larger than
SQLite's SQLITE_MAX_VARIABLE_NUMBER (32766) crashes inside chromadb.

Fetch drawers in batch_size=5000 chunks, stepping offset until the
collection is drained. by_source aggregation semantics are preserved
exactly — grouping, wing filter, meta capture all unchanged.

Closes MemPalace#1073. Related: MemPalace#802, MemPalace#850, MemPalace#1016.
tool_kg_query (as_of), tool_kg_add (valid_from), and tool_kg_invalidate
(ended) accepted any string and forwarded it to SQLite without format
validation. Parameterized queries prevent SQL injection, but invalid
date strings silently produce empty result sets — callers cannot
distinguish "no fact at this time" from "your date format was
unrecognized." This is especially painful for natural-language LLM
callers that synthesize dates like "March 2026" or "Jan 2025".

Add sanitize_iso_date() in config.py alongside the other input
validators. It accepts YYYY, YYYY-MM, and YYYY-MM-DD forms; passes
through None/empty; and raises ValueError with a field-named message
on anything else. Call it from the three kg MCP tool wrappers before
values reach the storage layer so the caller gets a clear error
instead of a silent miss.

Closes MemPalace#1164
Per qodo-ai review on PR MemPalace#1167: sanitize_iso_date() previously accepted
YYYY and YYYY-MM, but KnowledgeGraph.query_entity() compares valid_from/
valid_to TEXT columns lexicographically against as_of. Lexicographic
comparison treats '2026-01-01' as greater than '2026' (because '-' >
end-of-string), so partial as_of values silently excluded valid facts —
re-introducing the silent-empty-results problem this PR was meant to
fix.

Tighten _ISO_DATE_RE to require YYYY-MM-DD only. Update docstring and
error message accordingly. Invert the two test cases that asserted
partials were accepted.
Rollback cleanup was instantiating a fresh ChromaBackend, so the live backend that had opened the PersistentClient could keep file handles alive during restore. Close the active backend instance instead so rollback and CLI recovery can release Windows-safe locks before copying the backup back into place.
When the user removes ~/.mempalace/ (a strong "do not auto-capture"
signal), the next hook fire would silently recreate the entire dir
hierarchy and ingest existing transcripts:

1. _log() at hooks_cli.py:148 unconditionally calls
   STATE_DIR.mkdir(parents=True, exist_ok=True), so the act of
   writing the hook log line recreated ~/.mempalace/hook_state/
2. With no config file present, hook_stop_auto_save and
   hook_precompact_auto_save defaulted to True (no override to read)
3. The full save path then ran, materializing palace/, wal/,
   knowledge_graph.sqlite3, and N drawers from existing transcripts
   in ~/.claude/projects/*.jsonl

All four entry points (hook_stop, hook_precompact, hook_session_start,
and _log itself) now check a new PALACE_ROOT = Path.home() / ".mempalace"
constant first and short-circuit (returning {} on stdout, never logging)
when the dir is absent. The user-removable directory is now a kill-switch.

Five unit tests in tests/test_hooks_cli.py cover: hook_stop /
hook_precompact / hook_session_start do not create the dir when absent;
_log() does not create it when absent; existing dir proceeds normally
(regression).

Caught in the wild on a downstream fork: ~146 drawers materialized in
under a second after a deliberate `rm -rf ~/.mempalace/`, into a planning
session that was explicitly not meant to be captured.
Both @igorls and the Qodo bot flagged that `_palace_root_exists()` used
`Path.exists()`, which returns True for a regular file. A stray file at
`~/.mempalace` would let the kill-switch be bypassed and crash later in
`STATE_DIR.mkdir()` with NotADirectoryError.

Switched to `Path.is_dir()`. Also fold `_log()`'s inline check through
`_palace_root_exists()` so both kill-switch sites use the same predicate.

New test pins the behavior: a regular file at the palace root path is
treated as absent (hook short-circuits, _log does not crash, the stray
file is left untouched).
…1136)

Swap the module-level KnowledgeGraph singleton for a lazy, per-path
cache keyed by the resolved sqlite path. Import no longer creates a
sqlite file as a side effect, and MCP servers started with --palace
now route KG calls to the correct tenant when MEMPALACE_PALACE_PATH
changes between calls, matching the per-call behavior of _get_client()
on the ChromaDB side.

Default-path behavior is preserved: without --palace at startup, KG
stays on DEFAULT_KG_PATH regardless of env var. The "no --palace but
env var set" case is #540's scope and is not changed here.
Direct module-attribute patching of _kg is obsolete after the lazy
cache refactor. Switch test helpers to patch _get_kg instead so the
fixture KG replaces the factory rather than a now-missing singleton.

- tests/test_mcp_server.py: _patch_mcp_server helper
- tests/benchmarks/test_mcp_bench.py: _patch_mcp_config helper
- tests/benchmarks/test_memory_profile.py: inline patch in test_tool_status_repeated_calls
TestKGLazyCache covers the scenarios behind the lazy per-path refactor:

- test_lazy_init_no_import_side_effect: a fresh subprocess import does
  not create ~/.mempalace/knowledge_graph.sqlite3 (what closed PR #167
  was aiming at).
- test_get_kg_returns_same_instance: two _get_kg() calls under the same
  resolved path return the same object, cache has one entry.
- test_get_kg_different_paths_different_instances: rotating env var
  produces distinct KGs.
- test_multi_tenant_env_switch: the exact scenario from MemPalace#1136 — write
  under path A, query under path B returns empty, switching back to A
  sees the fact.
- test_cache_thread_safe: 16 threads racing _get_kg() end up with one
  shared instance and one cache entry.
Passing a stripped env dict without SYSTEMROOT/WINDIR breaks Python
bootstrap on Windows (_Py_HashRandomization_Init). Inherit the parent
env and strip MEMPAL* vars instead, then override HOME/USERPROFILE to
the tmp dir.
Inline comments referencing MemPalace#1136 and #540 add no information the
identifiers do not already convey. PR description carries the context;
code stays quiet.
tool_reconnect cleared ChromaDB caches but left _kg_by_path entries
intact. After an external replacement of knowledge_graph.sqlite3 the
server kept serving the old open sqlite3.Connection, returning stale
results.

Now iterate _kg_by_path under _kg_cache_lock, call close() best-effort,
and clear the dict so the next tool call reopens the KG from disk.
Two new tests in TestKGLazyCache verify cache invalidation and that a
failing close() does not block the clear.
…emPalace#1067)

ChromaBackend.close_palace() and close() evicted cached PersistentClients
from self._clients without calling client.close(), so chromadb 1.5.x kept
the rust-side SQLite file lock until GC. Reopening the same palace path
after shutil.rmtree + re-create within one process then failed with
SQLITE_READONLY_DBMOVED (SQLite code 1032).

Add _close_client() helper with a try/except fallback for older chromadb,
and route close_palace(), close(), and the DB-file-missing invalidation
branch of _client() through it. The mtime/inode auto-invalidation branch
is left as-is: callers there may still hold a live ChromaCollection
handle, and closing out from under them clears the rust bindings mid-use.

Regression tests cover close_palace reopen-same-path and whole-backend
close for multiple palaces.
The `python -m mempalace.fact_checker --stdin` entry point reads non-ASCII
text through the system ANSI codepage (cp1252/cp1251/cp950) on Windows,
which mojibakes characters before claim-extraction sees them. Reconfigure
stdin/stdout/stderr to UTF-8 with `errors="strict"`, wrapped in try/except
so a replaced stream (Jupyter, test harness) logs a warning rather than
crashing the CLI.

Mirrors the same fix shipped for `mcp_server.py:main()` (#400) and
`hooks_cli.py:run_hook()` (MemPalace#1280) -- this is the third and last
stdin-reading entry point in the package.
The primary `mempalace` console_script (`cli.py:main()`) reads non-ASCII
arguments via piped stdin and writes verbatim drawer text / wing names
through `print()`. On Windows, Python defaults stdio to the system ANSI
codepage (cp1252/cp1251/cp950), so:

- `mempalace search "..." > out.txt` mojibakes any drawer text containing
  non-Latin characters
- `mempalace ... < input.txt` mojibakes piped non-ASCII input

Reconfigure stdin/stdout/stderr to UTF-8 (`errors="strict"`) at the top
of `main()`, mirroring the helper added in this PR for fact_checker's
`__main__` block. Wrapped in try/except so a replaced stream (Jupyter,
test harness) logs a warning and continues rather than crashing the CLI.

The reconfigure cascades through every `mempalace` subcommand
(`init`/`mine`/`search`/`status`/`hook`/etc.) and through the interactive
flows that read non-ASCII names via `input()` (onboarding, entity
detector, room detector). With this commit the package's three
user-facing entry points (`mempalace`, `mempalace-mcp`, and
`python -m mempalace.fact_checker`) all reconfigure stdio identically on
Windows.
Previously all three streams reconfigured to UTF-8 with errors='strict'.
That kills 'mempalace search' the moment a drawer carrying a surrogate
half (round-tripped from a filename via surrogateescape) hits print(),
losing the rest of the result block. Same hazard for warning lines on
stderr.

Split the policy:
  stdin  -> surrogateescape (malformed bytes from a redirected file
            survive as lone surrogates instead of crashing the read)
  stdout -> replace (drawer text with a stray surrogate becomes U+FFFD
            instead of UnicodeEncodeError mid-print)
  stderr -> replace (same protection for logger / warning paths)

Applied identically in the cli.py and fact_checker.py helpers; the DRY
extraction into a shared module is a separate cleanup ask, kept out of
this fix to keep the diff narrow.

Tests updated for the new per-stream assertion.
Race scenario: a KG tool handler calls _get_kg() and gets a live
KnowledgeGraph; another thread fires tool_reconnect() between that
return and the handler's kg.add_triple()/kg.query_entity()/etc call.
tool_reconnect drains _kg_by_path and closes the underlying
sqlite3.Connection; the handler then raises sqlite3.ProgrammingError:
'Cannot operate on a closed database', which surfaces as a -32000
to the MCP client even though the user just asked for a reconnect.

New _call_kg(op) helper wraps each handler's kg call in a one-shot
retry: catch exactly sqlite3.ProgrammingError, evict the stale entry
(only if the cache slot still points at the closed instance — another
thread may have already replaced it), and rerun op against a fresh
_get_kg(). Beyond one retry give up so a sustained close-stream
surfaces clearly instead of looping.

All five KG handlers (tool_kg_query, tool_kg_add, tool_kg_invalidate,
tool_kg_timeline, tool_kg_stats) now route through _call_kg.

Tests pin the contract:
  * retries with a fresh KG and returns the second result
  * non-ProgrammingError exceptions propagate without retry
  * gives up after exactly one retry on sustained close
Both cli.py and fact_checker.py carried identical 28-line Windows stdio
reconfigure helpers; pull the loop into mempalace/_stdio.py so the same
machine drives the CLI, the fact_checker --stdin entry point, and the
MCP server. The thin per-call-site wrappers stay so existing tests keep
importing _reconfigure_stdio_utf8_on_windows from the same module they
always have.

CLI / fact_checker policy unchanged: stdin=surrogateescape (don't crash
on a malformed redirected file), stdout/stderr=replace (don't crash
mid-print on a surrogate half round-tripped from a filename).
…rash

EntityRegistry.save() called Path.write_text() directly, which truncates
the target file and then writes — so a crash mid-write (power loss, OOM,
filesystem-full mid-flush) leaves an empty or half-written
entity_registry.json. The whole people/projects map is lost; the system
falls back to an empty registry on next load.

Switch to the standard atomic-write pattern: serialize to a sibling
.tmp file in the same directory (so os.replace stays on one filesystem),
fsync, chmod 0o600, then os.replace over the target. The replace is
atomic on POSIX and Windows, so any crash leaves the previous registry
intact instead of a truncated file.

Tests cover: no leftover .tmp on success, and previous content preserved
when os.replace itself raises mid-save.
igorls and others added 20 commits May 7, 2026 10:41
Conflicts opened by MemPalace#1285 (temp-staging rebuild) and MemPalace#1312
(collection_name in recovery paths) merging after this branch was
authored.

mempalace/repair.py:
- Kept this branch's sqlite_integrity_errors() and
  print_sqlite_integrity_abort() helpers; took develop's rebuild_index
  signature with the collection_name parameter from MemPalace#1312. Normalized
  the helper's print indent to 2 spaces to match the rest of the file.

tests/test_repair.py:
- Kept both this branch's sqlite_integrity_errors tests and develop's
  rebuild_from_sqlite + configured-collection coverage.
- Replaced 7 sites of sqlite_path.write_text("fake") with
  sqlite3.connect(...).close() — write_text("fake") fails PRAGMA
  quick_check, so the new preflight aborts before the rebuild logic
  the tests actually exercise. An empty real SQLite DB passes
  quick_check and lets the tests run as intended.
- Took develop's temp-staging assertion shape (delete/create the
  __repair_tmp collection in addition to the live drawers collection)
  for the existing test_rebuild_index_success test.

Local: 1618 tests pass, ruff lint+format clean against 0.4.x CI pin.
…eq-id-preflight

fix(repair): preflight poisoned max_seq_id before rebuild (MemPalace#1295)
…-metadata-gate

fix(storage): quarantine partial HNSW flush without metadata (MemPalace#1274)
MemPalace#1357 (max_seq_id preflight) merged into develop while this branch
was in CI, opening a fresh conflict between the two preflight helpers.

mempalace/repair.py:
- Kept both: this branch's sqlite_integrity_errors() / print_sqlite_
  integrity_abort() AND develop's maybe_repair_poisoned_max_seq_id_
  before_rebuild() from MemPalace#1357. They check for distinct corruption
  classes and run as separate preflights.

tests/test_repair.py:
- Kept both this branch's sqlite_integrity_errors test group and
  develop's max_seq_id preflight test group; non-overlapping coverage.

Local: 1623 tests pass, ruff lint+format clean against 0.4.x CI pin.
…e-integrity-preflight

fix(repair): preflight SQLite integrity before rebuild
MemPalace#1364 added the SQLite quick_check preflight to rebuild_index, but
placed it AFTER backend.get_collection(...). On a SQLite-corrupt
palace, chromadb's rust binding raises pyo3_runtime.PanicException —
which is not a regular Exception subclass — so it propagates past the
existing `except Exception` handlers and the user sees a 30-line stack
trace instead of the friendly abort message MemPalace#1364 was designed to
deliver. Reproduced with `mempalace repair --yes` against a palace
whose chroma.sqlite3 has 4 mangled pages: pre-fix, panic; post-fix,
the clean abort message and exit code 1.

Two changes:

- mempalace/cli.py cmd_repair: run sqlite_integrity_errors() right
  after the basic palace-existence check, BEFORE the max_seq_id
  preflight (which itself opens sqlite3) and BEFORE backend =
  ChromaBackend(). Exit non-zero so unattended scripts and CI gates
  see the failure.

- mempalace/repair.py rebuild_index: same move at the function level
  for direct callers (tests, MCP) that bypass cmd_repair.

The new test test_rebuild_index_runs_sqlite_preflight_before_chromadb_open
uses a real chromadb-built palace (no ChromaBackend mock) plus a
real corrupt SQLite (16 KB of mangled pages) so the ordering is
exercised end-to-end. The previously-shipping test for the abort path
mocked both the backend and sqlite_integrity_errors, which is why the
ordering bug shipped CI-green.

Six existing test_cli.py cmd_repair tests used `(palace_dir /
"chroma.sqlite3").write_text("db")` to fake the SQLite file. The new
preflight correctly fails quick_check on those 2-byte stubs, so the
tests now create empty real SQLite DBs the same way the test_repair.py
fixtures already do.
Address Copilot review on MemPalace#1403: the test seeked unconditionally to
offset 40960 with only `pre_size > 16384` as a guard. If pre_size sat
between 16384 and 40960 + 16384 = 57344 (e.g., on a chromadb version
that allocated fewer pages on init, or a future schema change), the
seek would extend the file with zero-padding and the original pages
would stay intact — quick_check would still pass on the (untouched)
real data, and the regression guard would silently skip detecting a
preflight-ordering regression.

Compute the offset from pre_size, page-aligned, with explicit asserts
that the file is large enough to mangle 4 pages without truncating
the header or extending past EOF.
…-order

fix(repair): run SQLite integrity preflight before chromadb open (follow-up to MemPalace#1364)
The retry loop already backs off on HTTP 429/503 and rate-limit-shaped
exceptions, but JSONDecodeError exited on the first failure. Local LLM
runtimes occasionally produce malformed JSON (truncated streams, partial
chunks under load), and the retry was effectively dead for that path.

Mirror the 429/503 branch: sleep with exponential backoff and continue
through all 3 attempts, only returning None after the final failure.

Closes MemPalace#1155
A symlink pre-placed at the export output_dir or any wing subdirectory
would redirect markdown writes to wherever the symlink points. The
miner already rejects symlinked inputs via Path.is_symlink(); the
exporter should apply the same caution to outputs.

Add _reject_symlink() helper and call it before makedirs on both
output_dir and each wing_dir. Refusal raises ValueError with a clear
message rather than silently falling through.

Closes MemPalace#1156
The skip-if-unchanged check compared byte length only, so any in-place
edit preserving total length (typo fix "teh"→"the", word swap) was
silently dropped — a verbatim-storage violation: the user's actual
words never reached the palace.

Switch the gate to sha256(text). State entries gain a "content_hash"
field; the legacy size-only path is preserved when prev_hash is missing
so a post-upgrade run does not re-ingest every untouched diary.

Closes MemPalace#925
Address Copilot review on MemPalace#1156:

- Per-file symlink check via new _safe_open_for_write() helper. Uses
  O_NOFOLLOW on POSIX (close TOCTOU window between islink check and
  open) and falls back to islink + open on Windows. Applied to room
  files and index.md, mirroring the existing dir-level check.
- Tests now wrap os.symlink() in _try_symlink_or_skip() so Windows
  without Developer Mode and restricted CI sandboxes skip rather than
  hard-fail. Added two regression tests for the file-level cases
  (room file, index.md).
Address Copilot review on MemPalace#925:

- Full closet rebuild whenever the content hash differs from prior
  state, not only on entry-count growth. Without this, an in-place
  edit (same entry count, different body) updated the drawer but
  left the closet/search index stale — defeats the verbatim guarantee
  at the search layer even if the drawer is correct.
- Legacy size-only skip path now records the computed content_hash
  back into state so subsequent runs use the strict hash check
  instead of remaining on the size-only path indefinitely.
- Test updates: typo direction in the regression test now matches the
  comment (typo "Teh" → fix "The"), assertion now also checks the
  closet collection reflects the edit, and a new test exercises the
  legacy-state backfill path.
…try-on-json-decode

fix(closet_llm): retry _call_llm on JSONDecodeError (MemPalace#1155)
…ject-symlinks

fix(exporter): refuse symlinks at export targets (MemPalace#1156)
…sert

test_legacy_state_backfills_content_hash failed on test-windows because
Path.write_text without an encoding uses the system locale (cp1252 on
Windows). The em dash was written as 0x97, then read back by
diary_ingest as UTF-8 with errors=replace — round-trip produced
different bytes than the in-Python literal, so the assertion comparing
the persisted hash to sha256(text.encode(utf-8)) diverged.

Pin the write side to encoding=utf-8 so the on-disk bytes match what
diary_ingest decodes. No production change.
…t-hash

fix(diary): detect same-size edits via content hash (MemPalace#925)
113 commits since 2026-05-03 (1888b67 → ea36a00). 53 files changed,
+6077/-404. Conflict resolutions:

- .gitignore: keep both (ours + upstream's .envrc)
- README.md: keep fork README (per feedback_fork_readme_handling)
- mempalace/palace.py: take upstream logger name "mempalace_mcp"
- mempalace/cli.py: extend existing --mode choices to include
  from-sqlite; take upstream's --source / --archive-existing flags;
  drop upstream's duplicate --mode (HEAD already had one from row 23)
- mempalace/hooks_cli.py: keep our richer precompact docstring AND
  add upstream's _palace_root_exists short-circuit
- mempalace/searcher.py: keep delegate-to-search_memories CLI path
  (row 12) and drawer_id zip-with-id-pad (row 25); add upstream's
  defensive doc = doc or ""; harden _count_in_scope to coerce to
  Optional[int] so upstream's new mock-based tests pass against our
  BM25-fallback path
- tests/test_hooks_cli.py: keep our mock_ingest assertion + take
  upstream's 5 new absent-palace-root short-circuit tests
- tests/test_searcher.py: keep both fork's BM25-fallback test (row 12)
  and upstream's effective_distance_clamp test

Notable upstream items now in tree:
- f0d2360: our PR MemPalace#1377 (retry-once on _get_collection)
- 5134a63: SQLite integrity preflight before chromadb open
- 5488e7b: Windows mine hardening against ONNX bad_alloc
- a7c4ed2: --mode from-sqlite repair path
- 2ff6283: diary content-hash for same-size edits
- bddba59: 4 new auto-save tools/

Tests: 1733 passed, 1 skipped, 106 deselected (was 1591 pre-merge).
Records the upstream/develop sync from 1888b67 (2026-05-03) →
ea36a00 (2026-05-07, fork merge 1dbffe0). Tip is the v3.3.5-prep
develop branch.
Copilot AI review requested due to automatic review settings May 8, 2026 01:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

jphein added 4 commits May 7, 2026 20:26
Resolves the 4 pre-existing failures from `scripts/check-docs.sh`
check 5 (fork-only YAML commits referenced in CLAUDE.md):

- Row 21: add `f9f5cc4` (kind= filter add) and `7ba28dc` (retire) to
  the existing add→delete narrative.
- Row 23: add commit ref `42817d7` to the phase D 2026-04-26 update.
- Row 31: add commit ref `8252025` for the fork-side `scripts/deploy.sh`
  (was listed as needs-generalization but commit hash was missing).

Row 15 hedge cleanup: the "tracked as a follow-up" sentence about
`_get_session_recovery_collection` SIGSEGV exposure is moot — that
function no longer exists anywhere in the tree (verified via grep);
the recovery collection was retired entirely in row 21's deletion
and the verbatim-only shift (row 32). Replaced with a forward
reference instead of a tracking item.

Verified:
- `scripts/check-docs.sh` now passes all 5 checks (was 4 failures)
- `pytest -q` still 1,733 passed, 1 skipped — docs-only change
ruff F401: `pytest` imported but unused. The file uses no
`pytest.` calls — fixtures (tmp_path, monkeypatch) are pytest's
implicit injections, not pytest module references.

Pre-existing on main since 41359ba (2026-05-07, row 33's daemon
routing tests). Surfaced now because PR #15 is the first PR-
triggered CI run that touches the lint job since the import landed.

Same fix applies to PR #14's lint failure once #15 merges into
main and #14 rebases.

15/15 daemon tests still pass.
…i_daemon.py

CI's `ruff format --check .` flagged these two fork-only files as
unformatted. Same pre-existing class as the F401 in `0639b77` —
local pytest doesn't run ruff, so format drift was invisible until
CI surfaced it.

No semantic changes; pure whitespace/quote-style reformatting.
Pre-existing windows-only failures since rows 32 and 34 landed
(2026-05-05 and 2026-05-07). Surfaced now because PR #15 is the
first PR-triggered Windows CI run that exercised these.

1. tests/test_cli_daemon.py::test_routes_projects_mode_to_daemon
   `endswith("/home/u/proj")` failed because Path.expanduser()
   .resolve() produces "D:\\home\\u\\proj" on Windows. Normalize
   separators before the suffix check.

2. tests/test_normalize.py::test_config_reads_from_hooks_block
   monkeypatch.setenv("HOME", ...) doesn't redirect Path.home()
   on Windows (Path.home consults USERPROFILE on Windows, HOME on
   POSIX). Set both so the test redirect works on both platforms.

Both files PASS on Linux (no regression). Same fix would apply to
PR #14 once #15 merges into main.
@jphein
jphein merged commit 593160e into main May 8, 2026
6 checks passed
@jphein
jphein deleted the chore/sync-develop-2026-05-07 branch May 8, 2026 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.