chore: sync upstream/develop through v3.4.0 (2ec4bae) - #348
Merged
Conversation
Two bugs found in production use with a ChromaDB palace of 1200+ drawers ingested via mixed paths (bulk import + MCP tool calls): 1. searcher.py: filtered search (wing= or room=) crashes with "Error finding id" when the HNSW vector index is out of sync with the SQLite metadata store. The outer try/except swallowed the error as a search failure. Fix: inner try/except catches filter failures, retries unfiltered with n_results*15 (capped at 500), and post-filters by wing/room in Python. Degrades gracefully instead of returning an error. 2. mcp_server.py: diary_write requires 'entry' but add_drawer uses 'content', making it natural to pass content= by analogy. The mismatch returns a silent MCP -32000 error with no explanation. Fix: accept 'content' as an alias for 'entry' with a clear error message if neither is provided. Both bugs were diagnosed and patched in a live palace. This contributes the fixes upstream.
) chromadb <= 1.5.8 writes config_json_str = '{}' (empty JSON) when creating collections. chromadb 1.5.9 introduced a strict _type check in the collection config deserialization path -- its absence raises KeyError: '_type' on palace open. Since the pin allows >=1.5.4,<2, any upgrade pulls 1.5.9 and breaks every existing palace. Add a fourth pre-open migration step (_fix_missing_collection_type) that injects "_type": "CollectionConfigurationInternal" into collections.config_json_str rows that lack it. Same lifecycle and marker-file pattern as the existing _fix_blob_seq_ids. Co-Authored-By: nautis <nautis@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address review feedback: `with sqlite3.connect() as conn:` only manages transactions, it does not close the connection. An open connection before PersistentClient instantiation can leave WAL state. Use explicit `try...finally: conn.close()` matching the read-only helpers elsewhere in the module. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… for ChromaDB 1.5.x compatibility ChromaDB 1.5.x calls embedding_function.embed_query(input=...) via keyword argument during collection.query(). EmbeddinggemmaONNX lacked both embed_query and embed_documents methods, causing: TypeError: embed_query() got an unexpected keyword argument 'input' whenever semantic search was triggered. This patch adds the two methods required by the ChromaDB EF protocol, using (the ChromaDB kwarg name, noqa A002) so that palace search works correctly with the embeddinggemma model. Also downloads the companion ONNX file alongside the main model to prevent runtime InferenceSession failures. Fixes silent search failures when is set to embeddinggemma.
…ests) Adds first-class integration with Google's Antigravity IDE (https://antigravity.google/) as a third sibling to the existing Claude Code and Codex hook integrations. Strictly additive — no existing files in main are restructured. What ships ---------- * `.antigravity-plugin/` — verified-minimal plugin package: * `plugin.json` with `{"name": "mempalace"}` (no fabricated fields) * `mcp_config.json` registering the `mempalace-mcp` stdio server * `hooks.json.tmpl` templated with `__PLUGIN_DIR__` substitution * `skills/mempalace/SKILL.md` (real file — no symlinks) * `hooks/antigravity/`: * `lib/common.sh` — shared bash 3.2.57-compatible helpers with sentinel-guarded camelCase JSON parser, antigravity_*-namespaced state files, every existing kill switch, `MEMPAL_SAVE_INTERVAL >= 1` floor (no /0), and fail-open emitters * `mempal_save_hook_antigravity.sh` — Stop event handler: increments per-conversation counter, defers when fullyIdle=False or terminationReason=error, validates transcriptPath against `..` traversal, spawns `mempalace mine --mode convos` in a detached subprocess with a per-conversation pending marker, ALWAYS emits `{}` (never `{"decision":"continue"}` — that would force an infinite agent loop) * `mempal_wake_hook_antigravity.sh` — PreInvocation handler gated to invocationNum==1 with an atomic mkdir loop guard, runs `mempalace wake-up` with a 500ms hard timeout, emits verbatim output as `{"injectSteps":[{"ephemeralMessage":"..."}]}` or `{}` on any failure * `install.sh` — idempotent installer with cmp-gated copies, `__PLUGIN_DIR__` substitution, relative path absolutization, `--dry-run`, and basename-guarded `--uninstall` (refuses to wipe a directory whose basename isn't `mempalace`) * `INVESTIGATION.md` — verbatim quotes + URLs + dates from the five official Antigravity doc pages, recording every surface shipped and every surface deliberately omitted (PreCompact equivalent, slash-commands, rules/, plugin permissions field — the latter is third-party fabrication) * `STDIN_SHAPE.md` — exact stdin/stdout contract per event with worked examples * `README.md` — local hook docs + troubleshooting * `examples/antigravity/{hooks.json,mcp_config.json,README.md}` — standalone configs for users who don't want the full installer * `website/guide/antigravity.md` + sidebar entry — VitePress guide * Updates to `README.md`, `CHANGELOG.md` (Unreleased), `hooks/README.md` Tests (56 new, all passing) --------------------------- * `tests/test_antigravity_plugin_manifest.py` (11 tests) — schema contract on the in-repo `.antigravity-plugin/` directory, including guards against re-introducing the fabricated `permissions` field and against any symlink leak. * `tests/test_antigravity_hooks_shell.py` (31 tests) — invokes the bash hooks via subprocess with synthetic camelCase stdin, asserts `{}` on every failure path, kill-switch coverage (env vars + config.json + palace nuke), divide-by-zero floor, transcript traversal rejection, namespacing, wing inference, and the hard refusal to ever emit `decision=continue` from the Stop hook. * `tests/test_antigravity_hooks_install.py` (14 tests) — `--dry-run` side-effect-free, real install layout, executable bits preserved, byte-identical idempotent re-runs (md5 + filecmp), basename-match uninstall safety, refusal when plugin.json is missing or names a different plugin, relative path absolutization. Skipped on Windows. Verification ------------ * `uv run pytest tests/ --ignore=tests/benchmarks -v` → 2314 passed, 3 skipped (Windows), 1 unrelated warning * `uv run ruff check .` → all checks passed * `uv run ruff format --check .` → 139 files already formatted * `bash -n` clean on common.sh, both hook scripts, install.sh * Local install at `~/.gemini/config/plugins/mempalace/` verified end- to-end: layout correct, paths absolutized in hooks.json, both hooks fire with realistic camelCase JSON in <1s, wing inference picks `wing_mempalace` from workspacePaths[0], state files all `antigravity_*`-namespaced, second `install.sh` run produces byte-identical output (md5 snapshots match), uninstall removes only the mempalace plugin and leaves all 6 sibling Google plugins untouched. Constraints honoured -------------------- bash 3.2.57 (no mapfile / readarray / declare -A / `${var^^}`), verbatim guarantee on all wake injections, hooks <500ms / startup injection <100ms target (kill-switch path returns in <1.5s in CI), zero new runtime dependencies, no telemetry, no external API, strictly additive (existing Claude/Codex hooks unchanged). Refs: hooks/antigravity/INVESTIGATION.md for the full audit.
Five fixes for issues called out by the gemini-code-assist[bot] review. Each gets a regression test that locks in the correction. 1. CRITICAL: marker-cleanup watcher used POSIX `wait` on a sibling pid (save hook). bash `wait` only works on direct children of the calling shell — the `( wait $MINE_PID ... ) &` subshell runs as a sibling of MINE_PID, so wait fails immediately and the pending marker is deleted within milliseconds, defeating the concurrency guard. Replace with `while kill -0 $MINE_PID; do sleep 1; done`, which queries pid existence regardless of parent-child relationship. Test: test_save_hook_marker_watcher_uses_kill_polling. 2. Bare `mempalace` console-script invocation in the save hook fails when the venv's bin/ is not on the hook's PATH (e.g. uv tool install in some configurations, manually managed virtualenvs). Switch to `"$MEMPAL_PYTHON_BIN" -m mempalace mine ...` so the resolved interpreter runs the package directly via mempalace/__main__.py. Tests: test_save_hook_uses_python_module_invocation, test_save_hook_missing_mempalace_python_module_does_not_crash. 3. Same issue in the wake hook's inner Python helper. Switch `['mempalace', 'wake-up', ...]` to `[sys.executable, '-m', 'mempalace', 'wake-up', ...]` — sys.executable is the same interpreter that resolved MEMPAL_PYTHON in lib/common.sh. Test: test_wake_hook_uses_sys_executable_module_invocation. 4. The Python parser in lib/common.sh wrapped `json.load` in `try/except` and silently fell back to `data = {}`. The script then printed the `__MEMPAL_PARSE_OK__` sentinel even on parse failure, so the bash sentinel-check on the caller side (`[ "$_marker" != "__MEMPAL_PARSE_OK__" ]`) never triggered the defense-in-depth `input parse failed` branch. Remove the try/except so the exception propagates, Python exits non-zero, and the sentinel is omitted on bad JSON. The traceback still lands in antigravity_last_python_err.log for debugging. Test: test_common_sh_parser_omits_sentinel_on_malformed_json. 5. `mempal_save_interval()` failed to strip leading zeros from MEMPAL_SAVE_INTERVAL. Values like "08" or "09" then crashed the modulo step `$((COUNT % INTERVAL))` because bash arithmetic parses tokens starting with `0` as octal, and 8/9 are not valid octal digits ("value too great for base"). Strip leading zeros while preserving the literal "0" (which is then floored to 15). Test: test_save_hook_handles_leading_zero_save_interval (4 cases). Plus one cosmetic fix in install.sh: removed a no-op `(cd "$OLDPWD" 2>/dev/null || cd .) >/dev/null 2>&1` line in mempal_absolutize(). The subshell cd doesn't affect the parent shell, and the installer never cd's in the main shell anyway, so $PWD is already correct. Verification: * 9 new regression tests, all 65 antigravity tests pass * full repo: 2323 passed (was 2314), 3 skipped, 1 unrelated warning * ruff check + ruff format --check both clean across 139 files * bash -n clean on all four shell files * clean reinstall to ~/.gemini/config/plugins/mempalace/ succeeds * idempotent re-run produces zero file writes (cmp-gated) * both hooks return {} exit 0 with synthetic camelCase stdin Co-authored-by: Cursor <cursoragent@cursor.com>
Bumps [ruff](https://github.com/astral-sh/ruff) from 0.15.14 to 0.15.15. - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](astral-sh/ruff@0.15.14...0.15.15) --- updated-dependencies: - dependency-name: ruff dependency-version: 0.15.15 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
…has multiple parent_drawer_id mining passes
Under the additive-mining model (drawer history preserved across re-mines),
a single source_file can have multiple parent_drawer_id groups in the
palace, one per mining pass. Each group carries its own stored
source_mtime and normalize_version.
`file_already_mined` previously used `collection.get(where={"source_file": X},
limit=1)` in the `extract_mode is None` branch and only checked the single
returned row's metadata. ChromaDB's `get(..., limit=1)` has no ordering
guarantee across multiple matching rows, so the returned row was effectively
arbitrary. When ChromaDB happened to return a stale group (older mining
pass with a different stored mtime), the function returned False, the
additive miner concluded the file had changed, and wrote yet another
duplicate group of drawers for a file that had not actually changed since
the last successful mine.
Steady-state failure: for any source_file that has ever been edited (so
that the palace contains groups with different stored mtimes), each re-mine
has a probability of spuriously concluding "file changed" and writing
another duplicate group. Each spurious group compounds the problem because
its stored mtime can also trigger the next spurious re-mine. Storage grows
without bound; search results, hallway counts, and entity-frequency stats
become inflated proportionally.
The fix mirrors the paginated-iteration pattern already used in the
`extract_mode is not None` branch — iterate every drawer for the
source_file in 1000-row pages, short-circuit on the first matching group.
A correct group is one that passes the existing checks: normalize_version
not stale; if check_mtime, stored source_mtime within 0.001 seconds of the
current file mtime. The two branches (extract_mode is None vs set) collapse
into one loop that skips the extract_mode check when no extract_mode was
specified.
Trade-off: average-case cost rises from O(1) limit=1 query to O(N/1000)
paginated scan. For typical sources (1-3 groups) the cost is unchanged
because the loop short-circuits on the first matching group within the
first page. For pathological sources with thousands of groups, the cost
is O(number-of-pages-until-match) — still bounded, no longer flaky.
RED test pins the failure space deterministically
`test_file_already_mined_handles_multiple_groups_under_one_source_file`
uses a MockCollection that simulates two parent_drawer_id groups under one
source_file: a STALE group (older mtime) and a CURRENT group (matching
mtime). The mock returns the STALE group when called with limit=1
(worst-case ChromaDB ordering) and returns BOTH groups when called with
limit=1000 (what a correctly-iterating implementation must do). Test asserts
file_already_mined returns True even when limit=1 picks the stale group.
- Against pre-fix code: test FAILS (function returns False because
limit=1 picks stale group, mtime mismatch returns False)
- Against post-fix code: test PASSES (iteration finds the current group,
short-circuits to True)
Verified RED-then-GREEN locally. Existing 4 file_already_mined tests in
tests/test_miner.py continue to pass:
- test_file_already_mined_check_mtime
- test_file_already_mined_scopes_convo_extract_mode
- test_file_already_mined_extract_mode_paginates_large_sources
- test_file_already_mined_returns_false_for_stale_normalize_version
Verification
- macOS Python 3.12 (local) full pytest : 2268 passed, 0 failed
- Linux Python 3.9.25 (OrbStack) : 2260 passed, 0 failed
- Linux Python 3.11.15 (OrbStack) : 2261 passed, 0 failed
- Linux Python 3.13.13 (OrbStack) : 2261 passed, 0 failed
- ruff check + ruff format --check : all clean
Provenance
Surfaced during the per-query audit on the PR MemPalace#1628 amendment cycle (the
search for every bare `where={"source_file": ...}` query in the repo).
One of six sites identified. The other five are legitimately file-global
in intent (closet purges, full-rebuild deletes, paginated mode-filtered
scans). This site is the one whose failure mode mirrors the cross-group
stitching pattern PR MemPalace#1628 fixed at the searcher layer.
…ed-multi-mtime-groups fix(palace): file_already_mined iterates all groups when source_file has multiple parent_drawer_id mining passes
…0.15.15 build(deps-dev): bump ruff from 0.15.14 to 0.15.15
…tate-file GC Addresses igorls' review on PR MemPalace#1633 (antigravity branch only): - Atomic counter write: add mempal_write_counter_atomic (same-dir temp + mv -f rename) and use it in the save hook, replacing the truncate-then -write printf that the comment falsely called "atomic". Concurrent readers now always see a complete value. - Background the expensive probe: `mempalace --version` pays the full chromadb/onnx cold-start import (the mine subparser imports mempalace.miner before argparse handles --version), so running it in the foreground blew the <500ms save budget. Probe + mine + pending -marker cleanup now run in one detached subshell; the foreground returns immediately. This also retires the kill -0 watcher (the prior gemini fix) since cleanup is now sequential within the mine's own shell, removing the sibling-PID hazard entirely. - State-file GC: add mempal_state_ttl_days (default 30, env MEMPAL_STATE_TTL_DAYS) and mempal_gc_stale_state, a daily-throttled sweep (antigravity_last_sweep marker) that removes stale antigravity_save_count_*, antigravity_pending_*, and antigravity_woke_* artifacts. Called after the kill-switch check so a disabled hook touches nothing; specific name globs leave shared logs untouched. Tests: atomic-counter behavior + no-temp-leftover, single-subshell structure (no wait/kill -0/MINE_PID), backgrounded-probe timing (3s stub returns in <2s), async log polling for the missing-module path, GC sweep/throttle/TTL-validation, and GC gated by the kill switch. Also hardens the wake-missing test to pin MEMPAL_PYTHON so a shell -exported interpreter can't defeat the simulation. bash 3.2.57 safe, fail-open on every path, {}-only Stop output. Co-authored-by: Cursor <cursoragent@cursor.com>
…n-str-type fix(backends): repair missing _type in collection config (MemPalace#1611)
…eights + tokenizer) The EmbeddinggemmaONNX lazy-load now fetches the ONNX external-weights file (model.onnx_data) in addition to the model graph and tokenizer, so a single warm-up issues 3 downloads, not 2. The lazy-load-once invariant is unchanged (InferenceSession and Tokenizer.from_file are still each built exactly once).
Windows exports of Claude Code JSONL sessions prepend a UTF-8 BOM (\xef\xbb\xbf). With encoding='utf-8', json.loads() raises JSONDecodeError on the first line, _try_claude_code_jsonl silently skips every line, and the file falls through as raw text — losing all structured message content. utf-8-sig strips the BOM transparently and is backward-compatible with BOM-free files on all platforms.
…ace#1034) GBK consoles (Windows PowerShell/CMD default) cannot encode U+2713 (✓), U+2717 (✗), and U+2014 (—). The same class of UnicodeEncodeError fixed in miner.py via MemPalace#681 affects closet_llm.py and cli.py. Replace with ASCII equivalents: [OK], [FAIL], [!], and hyphen.
_chunk_by_exchange stripped every line, joined them with single spaces, and silently dropped blank lines. That violated the verbatim-always principle stated in CLAUDE.md and contradicted the function's own docstring, which claimed 'The full AI response is preserved verbatim.' Concrete consequences before this change: - paragraph breaks fused: 'para1\n\npara2' → 'para1 para2' - list items fused: '1. a\n2. b' → '1. a 2. b' - code fences destroyed: indented code collapsed to a single line - search quality degraded because tokenization changed at ingest Fix is surgical: keep each line as-is, join on newline, trim only trailing newlines produced by the loop stopping at the next '>' turn. The fallback path _chunk_by_paragraph has a narrower version of the same bug (it strips each paragraph); that is out of scope here and left for a follow-up.
…st (MemPalace#1579) _HNSW_BLOAT_GUARD set batch_size and sync_threshold to 50,000 to prevent link_lists.bin sparse-file bloat in pre-1.5.x Python chromadb (#344). chromadb >=1.5.4 Rust bindings do not exhibit that bloat. The 50k guard meant any mine under 50,000 drawers never triggered chromadb's _persist(), leaving index_metadata.pickle absent and link_lists.bin empty. quarantine_stale_hnsw then renamed the segment on every cold open after a 300s mtime gap, accumulating .drift-* directories indefinitely. Lower both thresholds to 2 (empirical Rust-side minimum; 1 is rejected with InvalidArgumentError) so any mine of 2+ drawers triggers a natural persist. Verified: batch_size=2 with 20k records produces link_lists.bin at 171 KB with zero sparse-file inflation. Existing palaces retain the old 50k thresholds in their collection metadata until the user runs repair --mode from-sqlite. Co-Authored-By: Tim Harmon <tim-harmon@users.noreply.github.com>
… data size chromadb pre-allocates data_level0.bin at index creation (~168 KB for 384-dim embeddings) regardless of record count, so the previous data-size-vs-floor heuristic in _segment_appears_healthy could not distinguish a single-record segment (sub-threshold, never persisted) from an interrupted persist. Restructure _segment_appears_healthy: when index_metadata.pickle is absent, check link_lists.bin instead of data_level0.bin size. Empty or absent link_lists + absent metadata = sub-threshold (never persisted). Non-empty link_lists + absent metadata = interrupted persist. Co-Authored-By: 0xKingVee9527 <0xWinner98@users.noreply.github.com>
…it reconnect (MemPalace#1573) The _quarantined_paths gate fired once per palace per process and never re-armed after external in-place writes (closet_llm, mine, compress) that drift HNSW segments. The MCP server path (make_client static) had zero discard logic -- quarantine never re-armed even on inode change. Extend the discard guard in _client() from inode_changed-only to inode_changed or mtime_changed or mtime_appeared. Add a guarded discard in mcp_server._get_client() before make_client(), and an unconditional discard in tool_reconnect(). Remove dead _auto_repair / palace-daemon comment (does not exist in this codebase) and correct misleading _get_collection retry-path comments that overclaimed quarantine re-runs. Co-Authored-By: C-LaForest <C-LaForest@users.noreply.github.com>
Prevent redundant quarantine re-run when a fresh ChromaBackend instance opens a palace that was already quarantined by another instance in the same process. The mtime_appeared transition (cached 0.0 -> real mtime) now only triggers a discard if the instance previously tracked the path, distinguishing genuine file appearance from first-access default. Addresses gemini-code-assist review on PR MemPalace#1602. Co-Authored-By: C-LaForest <C-LaForest@users.noreply.github.com>
Drawer/closet/triple IDs built by concatenating strings without a
delimiter before hashing can collide: hash((s1 + str(i1))) ==
hash((s2 + str(i2))) whenever s1+str(i1) == s2+str(i2). Concretely,
source_file="foo" + chunk_index=12 produces the same hash input as
source_file="foo1" + chunk_index=2 — both yield "foo12". Under
ChromaDB's primary-key constraint the second write silently
overwrites the first; one chunk's content is lost without surfacing
an error (the surrounding broad `except Exception:` blocks swallow
any backend complaint).
The defect class is "string + string concat fed to a hash without
a delimiter." Per the styleguide's partial-scope-key-migration rule,
every site sharing the pattern is a candidate and must be triaged —
not just the ones the original issue named.
FIX — 6 sites
- mempalace/miner.py:1253 drawer_id, add_drawer() back-compat
- mempalace/miner.py:1386 drawer_id, batched mine loop
- mempalace/miner.py:1416 drawer_ids, closet-build re-derivation
- mempalace/format_miner.py:643 drawer_id, format_miner batched loop
- mempalace/mcp_server.py:1136 drawer_id, tool_add_drawer
- mempalace/knowledge_graph.py:305 triple_id, KG triple insertion
MIGRATED FOR CONSISTENCY — 2 sites
- mempalace/convo_miner.py:87 sentinel_key — was `:`, now `|`
- mempalace/convo_miner.py:422 drawer_key — was `:`, now `|`
Pre-existing delimiter was `:`. Migrated because (a) source_file can
contain literal `:` on Windows paths (`C:\Users\…`) and URL-like
sources (`https://host:8080`), weakening `:` as a separator; (b) `|`
is reserved in Windows filenames so source_file cannot contain it,
making it strictly safer.
DELIMITER CHOICE
- `|` chosen over `:` (Windows / URL source_file edge cases)
- `|` chosen over `\x00` (NUL breaks log greppability + print()
debug + downstream string handling for marginal extra safety)
- Matches the established precedent in mempalace/diary_ingest.py
(lines 52, 76, 91, 98 — all already on `|`)
EXEMPT — audited and correct as-is
Single-input hashes (nothing to delimit):
- mempalace/miner.py:1432 closet_id (source_file only)
- mempalace/format_miner.py:559 sentinel_id (source_file only)
- mempalace/palace.py:433 lock filename (source_file only)
- mempalace/palace.py:629 palace_key (lock_key_source only)
- mempalace/diary_ingest.py:158 content_hash (text only)
- mempalace/hooks_cli.py:329 pidfile digest (joined cmd only)
- mempalace/sources/context.py:141 record digest (source_file only)
Already correctly delimited:
- mempalace/hallways.py:157 `f"{wing}::{a}::{b}"` (`::`)
- mempalace/palace_graph.py:454 `f"{a}↔{b}"` (`↔`)
- mempalace/diary_ingest.py:52,76,91,98 (`|` precedent)
Protected by composition (uniqueness guaranteed by the ID prefix,
not by the hash slice):
- mempalace/mcp_server.py:1635 entry_id is
`diary_{wing}_{strftime('%Y%m%d_%H%M%S%f')}_{sha[:12]}`.
Microsecond-resolution timestamp prefix supplies uniqueness;
the trailing hash is a content-discriminator, not the
write-time uniqueness guarantor.
NEW MODULES
- mempalace/ids.py — single source of truth for ID construction.
Five helpers (make_drawer_id_from_chunk, make_drawer_id_from_content,
make_convo_drawer_id, make_convo_sentinel_id, make_triple_id) plus
an ID_RECIPE = "v2" constant.
- mempalace/collision_scan.py — pre-mining defense. Runs immediately
before each batched ChromaDB upsert; raises CollisionError naming
the colliding (source_file, chunk_index) pairs if any proposed
drawer_id appears more than once with conflicting metadata across
the union of incoming and existing rows.
DETECTION
ChromaDB's primary-key constraint means past collisions were
silently resolved at upsert time (last-write-wins), erasing
evidence. A post-hoc audit cannot reconstruct what was lost from
palace state alone. Therefore:
- Pre-mining risk scan. Before each batched upsert, compute the
proposed drawer_ids for the incoming chunk set AND query existing
drawer_ids from the collection. If any proposed id appears more
than once in the union (incoming-vs-incoming or incoming-vs-
existing) with conflicting (source_file, chunk_index), abort the
mine with an actionable error naming the colliding pairs.
Collision is caught BEFORE it destroys data, which is the only
point at which palace state still carries the evidence.
- New metadata key: `"id_recipe": "v2"` on every drawer written
under the delimited recipe. Audits compare like-for-like;
drawers without `id_recipe` are treated as v1 legacy (undelimited
or `:`-delimited), not as collisions.
- Honest disclosure: palaces mined under any pre-v2 mempalace may
carry silent past collisions whose original content is
unrecoverable from palace state. Future library tier work will
give users a per-drawer audit + opt-in archival path.
TESTS
- tests/test_ids.py: 17 unit tests covering all 5 helpers, the
ID_RECIPE constant, the private `_delimited_sha256` helper, and
the four defect-class collision shapes (chunk_index boundary,
content boundary, extract_mode boundary, ISO datetime boundary).
RED collision tests confirm the v2 recipe breaks the defect class.
- tests/test_collision_scan.py: 10 tests covering clean batches,
idempotent re-mines, incoming-vs-incoming collisions, incoming-vs-
existing collisions, error-message quality, empty batches,
metadata without chunk_index, and ChromaDB backend errors
propagating cleanly.
- tests/test_convo_miner_unit.py: FakeCol gains a stub `get()` so
the pre-mining scan can probe an empty in-test collection.
BACKWARDS COMPATIBILITY
- Existing v1 drawers remain queryable; no rewrite of stored IDs.
- New mining passes write v2 drawers alongside v1 via PR MemPalace#1628's
additive-mining model.
- No user action required; opt-in cleanup ships separately.
VERIFICATION
- macOS Python 3.12 (local): 2304 passed, 3 skipped, 0 failed,
coverage 85.44% (above 80% threshold).
- Linux Python 3.9/3.11/3.13 (OrbStack with full `pip install -e
'.[dev]'` flow): see PR body for per-version pass counts.
- `ruff check .` + `ruff format --check .`: clean.
- pylint score: ids.py 10.00/10, collision_scan.py 10.00/10; all
modified files >= 8.92 (no regression).
- bandit: clean on all new files; the one MEDIUM finding in
knowledge_graph.py is on lines 385/407 (pre-existing SQL string
construction in timeline queries), not in code touched by this PR.
- pre_push_check.sh: ALL CHECKS PASSED.
Refs: deferred from PR MemPalace#1572 (Copilot finding #13). Styleguide audit
documented above: 8 fix/migration + 11 enumerated exempt sites.
`uv tool install mempalace` / `pipx install` place the mempalace console scripts in an isolated environment whose interpreter is not the system python3. mempal_resolve_python previously resolved `command -v python3`, landing on a Python that cannot import mempalace: the `-m mempalace --version` probe failed and mining silently never fired (hit by a real user on PR MemPalace#1633). Resolution now derives the interpreter from the mempalace-mcp / mempalace console-script shebang on PATH (the same script the MCP server launches) before falling back to python3. It is pure shebang parsing + stat — no Python subprocess at source time — so the hook performance budget is preserved. An env-style `#!/usr/bin/env python` shebang and a non-executable interpreter are both rejected and fall through. MEMPAL_PYTHON remains the explicit override. Adds 6 resolver regression tests, documents resolution + MEMPAL_PYTHON in the guide and hooks README (fixing the stale `command -v mempalace` note), and a CHANGELOG entry. Co-authored-by: Cursor <cursoragent@cursor.com>
…hokepoint MemPalace#1235 sanitised lone UTF-16 surrogates for the MCP write tools, but the bulk ingest paths (miner, convo_miner, sweeper, diary_ingest) build documents without routing through sanitize_content() and reach ChromaCollection directly. A single lone surrogate in document text raises UnicodeEncodeError inside chromadb and aborts the whole add/upsert batch with a -32000 Internal Error, silently dropping every other row in the same batch. Complete the chokepoint: add _sanitize_documents_for_chromadb (mirror of _sanitize_metadatas_for_chromadb) and apply it in add/upsert/update so the backend guarantees UTF-8-safe documents regardless of caller. IDs and dedup are unaffected (IDs are computed upstream); only illegal lone surrogates become U+FFFD, matching the errors="replace" behaviour used elsewhere.
Per Gemini review on MemPalace#1673: chromadb accepts OneOrMany[Document], so a bare str document was iterated character-by-character by the list comprehension, splitting it into per-character documents (the silent corruption this method exists to prevent). Handle isinstance(str) explicitly; add a regression test.
A path-encoded dirname like `-home-user-proj` produced a leading-underscore slug (`_home_user_proj`) that sanitize_name — and therefore the MCP write tools — reject, so the conversation miner filed transcripts into wings the MCP could never write to. Strip leading/trailing `_` after collapsing separators so the slug is valid. Adds tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…emPalace#1676) The write-ahead-log directory was created at module scope in mempalace/mcp_server.py, so importing the MCP server ran `_WAL_DIR.mkdir(parents=True, exist_ok=True)` and recreated `~/.mempalace` even after a user removed it to engage the documented kill-switch (`hooks_cli._palace_root_exists()`, MemPalace#1305). On every session start this re-armed the autosave/mining hooks the user had disabled. Move the WAL directory setup into a lazy `_ensure_wal()` called from the write path (`_wal_log`). Importing the module no longer touches disk; the directory is created on the first real write, when the palace is being written to anyway. The WAL is intentionally not gated on `_palace_root_exists()` (the ChromaDB/KG layer recreates the palace regardless, so gating would only drop audit records); runtime kill-switch enforcement for MCP writes is tracked in MemPalace#504. Add regression tests: a subprocess import asserts `~/.mempalace` is not created, and a write test asserts the directory is created lazily with the expected permissions. Co-Authored-By: Grace Gettert <9805362+ggettert@users.noreply.github.com>
feat(backends): observable maintenance hooks + pgvector indexed-build path (RFC 001)
…FC 001, MemPalace#1730) Completes the embedder-identity contract for the last backend (qdrant) and unifies identity persistence across all local-path backends. Identity is stored in a small per-palace sidecar (mempalace_embedder.json), NOT in a backend's mismatch marker. The marker's presence signals "palace initialized" (reads raise CollectionNotInitializedError when the marker exists but the store doesn't), so recording identity at first empty open must not create it — a sidecar is unguarded, so a brand-new palace records identity immediately. This fixes a latent gap (caught in review) where pgvector/qdrant palaces stayed permanently "unknown" because the marker isn't written until the first real write. - New mempalace/backends/_sidecar.py: shared read/write_embedder_sidecar with the isinstance robustness the review bots taught us; chroma, pgvector, and qdrant all use it (chroma's inline copy is removed, pgvector switches off the marker, qdrant adds it). - QdrantCollection.get/set_embedder_identity delegate to the sidecar, so palace.get_collection enforces a model swap on a qdrant palace exactly like the other backends — no live server needed for the check. Tests: sidecar roundtrip + brand-new-palace recording (creates sidecar without a marker) + enforcement model-swap raise, for qdrant and pgvector, all server-free; chroma identity tests unchanged. Full suite: 2495 passed, 82.52%. Closes MemPalace#1730. Refs MemPalace#743, MemPalace#1724.
LLM clients (Claude, Codex) often autocorrect acronyms in tool args (ps5→PS5). When the only difference is casing, treat it as a no-op to prevent silent room fragmentation. Genuinely different room/wing values still apply normally. Fixes MemPalace#1621
Addresses review: old_meta values could be non-string after corruption or external writes. str() prevents AttributeError.
Closes MemPalace#1739. Hallways shipped in 3.3.6 (MemPalace#1558) with Python API entry points `list_hallways(wing=None)` and `delete_hallway(...)` in `mempalace/hallways.py`, but neither was registered as an MCP tool in `mempalace/mcp_server.py`. Mining produces hallways visible in the mine log ("Hallways: +N within-wing entity link(s)") but they were not retrievable through MCP. This change wires both functions into the MCP tool registry mirroring the existing tunnel-tool pattern: - `mempalace_list_hallways(wing: str | None = None)` wraps `hallways.list_hallways`. Optional `wing` filter goes through `_sanitize_optional_name` just like `tool_list_tunnels`, so invalid names surface a structured error instead of crashing. - `mempalace_delete_hallway(hallway_id: str)` wraps `hallways.delete_hallway` and returns `{"deleted": bool}` so callers can distinguish a successful delete from a no-op. Tests in `tests/test_mcp_server.py` cover: - list returns all records without filter - list filters correctly by wing - list rejects invalid wing names with a structured error - delete removes the targeted record and returns `{"deleted": True}` - delete with an unknown id returns `{"deleted": False}` - delete with missing/non-string id returns a structured error - both tools are present in the public `TOOLS` registry Pure pass-throughs of the existing Python API. No behavior change in the underlying hallway storage, no schema change to the `hallways.json` file format.
CI's test_no_undocumented_tools enforces that every tool registered in the TOOLS dict has a corresponding section in mcp-tools.md. The two hallway tools from b866f41 were missing — adding them here. Sections mirror the format of the existing list_tunnels and delete_tunnel entries directly above.
…-identity feat(backends): shared embedder-identity sidecar + qdrant identity (RFC 001)
…eletions fix(backends): keep post-deletion dim-None HNSW segments instead of quarantining (MemPalace#1710)
fix(backups): add max_backups retention to bound backup disk usage
…tool feat(mcp): add mempalace_mine tool (MemPalace#1662)
…-skips-already-mined fix(miner): count only new work toward --limit, not already-mined skips (MemPalace#1535)
feat(mcp): expose list_hallways and delete_hallway tools
…rawer-room-casing fix(mcp): preserve room/wing casing on case-only update
…ity-support feat: add Antigravity IDE support (hooks, plugin, skill, docs, tests)
Adds the recall layer to the Antigravity plugin, mirroring the three- layer wiring from feat/cursor-hooks-support, adapted for Antigravity's native plugin and hook surfaces. The wake hook (PreInvocation, invocationNum==1) was already the eager layer — it injects verbatim palace content via injectSteps[]. ephemeralMessage before the first model call, which is strictly better than Cursor's static directive. This commit adds the on-demand layers: - integrations/shared/recall-protocol.md: single canonical source of truth for "search before answering", shared across Cursor, Antigravity, Claude Code, Codex, OpenClaw. Ported from feat/cursor-hooks-support with the wake-up step de-coupled from any IDE-specific hook name. - .antigravity-plugin/skills/mempalace-recall/SKILL.md: recall-only skill with Protocol, Tool selection, Unhappy paths, and Anti-patterns sections. Tailored to Antigravity: references injectSteps injection, not Cursor's additional_context; links to shared protocol via GitHub URL (relative file paths break in the installed copy at ~/.gemini/config/plugins/mempalace/). - .antigravity-plugin/rules/mempalace-recall.md: plain .md recall rule (no .mdc extension, no YAML frontmatter — per Antigravity's plugin docs which specify rules/<name>.md). Body mirrors Cursor's rule; references the shared protocol via absolute GitHub URL. - hooks/antigravity/install.sh: creates skills/mempalace-recall/ and rules/ in the install dir, copies both new files (cmp-gated, idempotent). Verified: first run writes both, second run is silent. - .antigravity-plugin/README.md: updated layout tree + "Three recall layers" section documenting wake hook (eager), recall skill (on-demand), and optional rule. - 11 new tests in tests/test_antigravity_plugin_manifest.py covering: shared protocol presence, recall skill structure (frontmatter, required sections, GitHub URL reference), rule format (plain .md, no frontmatter), and installer (mkdir + copy_file lines present). Co-authored-by: Cursor <cursoragent@cursor.com>
…urce Steps 2-6, unhappy paths, and anti-patterns now match integrations/shared/recall-protocol.md verbatim. Step 1 remains Antigravity-specific (PreInvocation injectSteps injection). Flagged by gemini-code-assist on PR MemPalace#1771. Co-authored-by: Cursor <cursoragent@cursor.com>
…ity-support feat(antigravity): add recall skill, optional rule, shared protocol
Advances the fork's upstream-tracking ancestry from v3.3.6 (6957c7e) to current develop (2ec4bae): 113 commits / ~40 PRs including the v3.4.0 release, the RFC-001 pluggable-backend stack (MemPalace#1679 + MemPalace#1727 metric-aware similarity, MemPalace#1731/MemPalace#1734 embedder identity, MemPalace#1732 maintenance hooks), wing-normalize (MemPalace#1675/MemPalace#1702, opt-in migrate-wings), delimiter-safe drawer ids (MemPalace#1666, new drawers only — verified safe for the 409K-drawer production palace), additive-mining idempotency fix, miner robustness (MemPalace#1137/MemPalace#1100/MemPalace#1102/MemPalace#1622/MemPalace#1602), and antigravity harness support. Key resolution decisions: - hooks_cli: diary checkpoints RESTORED per JP (2026-06-11) — the silent path writes a themed diary entry (agent_name-filed, MemPalace#1693) AND ingests the verbatim transcript; save marker advances only on confirmed diary write (original silent-mode contract) - palace.get_collection: unified — upstream backend resolution (explicit/config/env/markers + mismatch protection) + fork's postgres DSN options, KG write-through attach, and collection_name None/DEFAULT shim; embedder-identity enforcement + EmbeddingCollection wrap from upstream - mcp_server._get_collection: fork postgres branch + upstream generic non-chroma branch + fork chroma branch (RETIRED gate intact); 37 tools - searcher: fork pipeline kept (env-tunable hybrid weights, AGE fusion, multi-encoder, BM25 backend dispatch, tags) with upstream's metric-aware _distance_to_similarity threaded through _hybrid_rank - file_already_mined: upstream's additive-mining group scan (fixes limit=1 ordering re-mines) - version 3.4.0; CHANGELOG upstream-first then fork entries; fork README kept on main Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ated - cli: drop duplicated status subparser from the merge union - config: backend precedence aligned to RFC 001 (config.json beats MEMPALACE_BACKEND); init() no longer seeds a backend key - palace.get_collection: derive backend_name from resolve_backend_name (not the instance attr) so fakes/options keep working - searcher: union merger uses the RFC-001 lexical_search capability path (UnsupportedCapabilityError propagates) with the fork's postgres tsvector branch kept for production; CLI prints metric-labeled similarity; tags threaded through the vector-disabled fallback; leftover merge fragment removed - mcp_server: close-path uses get_backend_for_palace (PalaceRef API); handle_request noqa'd for merged fork+upstream dispatch complexity - tool count 34 -> 37 across all doc claims; README badges/sync line to v3.4.0 (2ec4bae), 4282 collected tests; fork-changes.yaml entry + FORK_CHANGELOG/llms-full/python-api regenerated; check-docs clean Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Secrets |  |
Jun 11, 2026 3:21p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request performs a major synchronization with upstream v3.4.0 and introduces significant architectural enhancements, most notably the RFC-001 pluggable-backend stack. It adds robust support for Google's Antigravity IDE, introduces new storage backends (pgvector, Qdrant, sqlite_exact), and improves operational safety through backup retention and embedder identity enforcement. The changes also include a migration utility for wing-name normalization and various robustness fixes for the mining and search pipelines. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughAdds Antigravity plugin packaging and hook runtime support, expands storage backends with backend-aware routing and identity handling, and updates release, Docker, versioning, and publishing metadata. ChangesAntigravity IDE support
Storage backends and backend-aware routing
Release, containers, and publication metadata
Sequence Diagram(s)sequenceDiagram
participant hooks_antigravity_install_sh as hooks/antigravity/install.sh
participant mempal_save_hook_antigravity_sh as mempal_save_hook_antigravity.sh
participant mempal_wake_hook_antigravity_sh as mempal_wake_hook_antigravity.sh
participant common_sh as hooks/antigravity/lib/common.sh
hooks_antigravity_install_sh->>common_sh: render hooks.json and copy hook scripts
mempal_save_hook_antigravity_sh->>common_sh: parse stdin and schedule periodic save
mempal_wake_hook_antigravity_sh->>common_sh: parse stdin and inject wake message
Changes🎯 5 (Critical) | ⏱️ ~120+ minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces first-class Google Antigravity IDE support, a pluggable-backend stack (supporting Chroma, pgvector, Qdrant, and SQLite-exact), backup retention pruning, and collision-safe drawer ID generation. The code review identified several critical issues: a bypass of the collision scan on non-Chroma backends due to missing dataclass attribute checks, thread-safety race conditions during connection initialization in both the SQLite-exact and pgvector backends, and a severe bug in the MCP server where globally redirecting standard output to capture miner progress hijacks the JSON-RPC stdio communication channel, causing the server to hang.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+88
to
+89
| existing_ids: list = result["ids"] if hasattr(result, "__getitem__") else [] | ||
| existing_metas: list = result["metadatas"] if existing_ids else [] |
There was a problem hiding this comment.
The collection.get call returns a GetResult dataclass on non-Chroma backends (such as sqlite_exact, pgvector, and qdrant), which does not implement __getitem__. As a result, hasattr(result, "__getitem__") evaluates to False, causing existing_ids and existing_metas to be silently set to empty lists. This completely bypasses the collision check against existing drawers for all non-Chroma backends.
To fix this, check for the dataclass attributes (ids and metadatas) first before falling back to dictionary-style lookup.
Suggested change
| existing_ids: list = result["ids"] if hasattr(result, "__getitem__") else [] | |
| existing_metas: list = result["metadatas"] if existing_ids else [] | |
| if hasattr(result, "ids"): | |
| existing_ids = result.ids | |
| existing_metas = result.metadatas | |
| elif hasattr(result, "__getitem__"): | |
| existing_ids = result["ids"] | |
| existing_metas = result["metadatas"] | |
| else: | |
| existing_ids = [] | |
| existing_metas = [] |
Collaborator
Author
There was a problem hiding this comment.
Checked — GetResult subclasses _DictCompatMixin (backends/base.py:324), which provides __getitem__, so the hasattr guard evaluates True for every backend's result and the scan does run. The guard exists for third-party backends returning bare objects. No change needed.
Comment on lines
+839
to
+851
| with self._clients_lock: | ||
| cached = self._clients.get(palace_path) | ||
| if cached is not None and not cached.closed: | ||
| return cached | ||
| conn = sqlite3.connect(db_path, check_same_thread=False) | ||
| conn.row_factory = sqlite3.Row | ||
| lock = threading.RLock() | ||
| handle = _SQLiteExactHandle(conn, lock) | ||
| with handle.lock: | ||
| self._init_schema(conn) | ||
| with self._clients_lock: | ||
| self._clients[palace_path] = handle | ||
| return handle |
There was a problem hiding this comment.
In _connect, the connection creation and schema initialization (_init_schema) are performed outside of the self._clients_lock block. If multiple threads attempt to connect to the same palace_path concurrently, they can race to initialize the schema on the same SQLite database file, leading to transient database is locked errors or duplicate connection handles overwriting each other in self._clients.
To ensure thread safety and prevent resource leaks, wrap the entire connection and schema initialization block inside the self._clients_lock context manager.
Suggested change
| with self._clients_lock: | |
| cached = self._clients.get(palace_path) | |
| if cached is not None and not cached.closed: | |
| return cached | |
| conn = sqlite3.connect(db_path, check_same_thread=False) | |
| conn.row_factory = sqlite3.Row | |
| lock = threading.RLock() | |
| handle = _SQLiteExactHandle(conn, lock) | |
| with handle.lock: | |
| self._init_schema(conn) | |
| with self._clients_lock: | |
| self._clients[palace_path] = handle | |
| return handle | |
| with self._clients_lock: | |
| cached = self._clients.get(palace_path) | |
| if cached is not None and not cached.closed: | |
| return cached | |
| conn = sqlite3.connect(db_path, check_same_thread=False) | |
| conn.row_factory = sqlite3.Row | |
| lock = threading.RLock() | |
| handle = _SQLiteExactHandle(conn, lock) | |
| with handle.lock: | |
| self._init_schema(conn) | |
| self._clients[palace_path] = handle | |
| return handle |
Collaborator
Author
There was a problem hiding this comment.
Confirmed real, but it's upstream code byte-identical to upstream/develop — this sync deliberately doesn't diverge from upstream inside the new backends. Filed upstream as MemPalace#1774 (concurrent first-open can double-create connections; the loser handle leaks).
Comment on lines
+493
to
+494
| rows = None | ||
| else: |
There was a problem hiding this comment.
In _execute, self._connect() is called outside of the self._lock block. Since _connect modifies and accesses self._conn without synchronization, concurrent queries from multiple threads can cause a race condition where multiple connections are opened concurrently, leading to connection leaks or desynchronization of the active connection handle.
To ensure thread safety, move the self._connect() call inside the with self._lock: block.
with self._lock:
conn = self._connect()
Collaborator
Author
There was a problem hiding this comment.
Confirmed real (shared-instance threading: _connect mutates self._conn outside self._lock), but byte-identical to upstream/develop — filed upstream as MemPalace#1775 rather than diverging in the sync. Our production shape (one connection per process via the daemon) isn't exposed, and the live conformance run used one backend per thread.
Comment on lines
+2276
to
+2284
| try: | ||
| with tempfile.TemporaryFile() as tmp: | ||
| os.dup2(tmp.fileno(), 1) | ||
| try: | ||
| with contextlib.redirect_stdout(buf): | ||
| result = fn() | ||
| finally: | ||
| sys.stdout.flush() | ||
| os.dup2(saved_fd, 1) |
There was a problem hiding this comment.
The _capture_fd_stdout helper redirects file descriptor 1 (standard output) globally for the entire process using os.dup2(tmp.fileno(), 1).
Since the MCP server communicates with the client via JSON-RPC over stdio, redirecting fd 1 globally will completely hijack and block the communication channel of the entire server during the mine operation. Any concurrent JSON-RPC responses or background notifications written to fd 1 by other threads will be redirected to the temporary file instead of being sent to the client, causing the client to hang or disconnect.
Instead of redirecting fd 1 globally, consider suppressing C-level output by configuring the log levels of the underlying libraries (such as onnxruntime and chromadb) directly, or run the mining operation in a separate subprocess where redirecting standard output is safe and isolated.
Collaborator
Author
There was a problem hiding this comment.
This helper is byte-identical to upstream's (_capture_fd_stdout exists in upstream/develop with the same body). The window is bounded: the stdio server dispatches requests serially, so no JSON-RPC write can interleave with the captured section, and fd 1 is restored in a finally. Agreed it's fragile if dispatch ever goes concurrent — that's an upstream design conversation, not a sync deviation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mempalace/miner.py (1)
1356-1373:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
add_drawers()on the same ID recipe as the other insert paths.
add_drawers()still hardcodes the legacydrawer_{wing}_{room}_{sha256(...)}format, whileadd_drawer()andprocess_file()now usemake_drawer_id_from_chunk(). Any caller of this batch API will mint different IDs for the same chunk, which breaks idempotent re-mines and makesid_recipeinaccurate for rows created through this path.Suggested fix
for chunk in chunks: - drawer_id = f"drawer_{wing}_{room}_{hashlib.sha256((source_file + str(chunk['chunk_index'])).encode()).hexdigest()[:24]}" + drawer_id = make_drawer_id_from_chunk( + wing, room, source_file, chunk["chunk_index"] + ) metadata = _build_drawer_metadata( wing, room,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/miner.py` around lines 1356 - 1373, The batch path in add_drawers() generates drawer IDs using a legacy hardcoded pattern, causing inconsistent IDs vs add_drawer()/process_file(); replace the manual id construction in the loop with a call to the shared helper make_drawer_id_from_chunk(...) (pass the same arguments used elsewhere: wing, room, source_file, chunk["chunk_index"] and any other required params) so batch_ids uses the same id recipe and preserves idempotency and accurate id_recipe. Ensure you remove the hashlib-based string assembly and use the helper's return value for batch_ids.mempalace/mcp_server.py (1)
1620-1630:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate the requested search strategy on the retry path.
Line 1620 retries
search_memories(...)after a transient index error, but it dropscandidate_strategyandfusion_mode. A request that started asvectororrrfcan therefore recover into the default ranking mode and return a different result set than the caller asked for.💡 Suggested fix
result = search_memories( sanitized["clean_query"], palace_path=_config.palace_path, wing=wing, room=room, tags=normalised_tags or None, n_results=limit, max_distance=dist, vector_disabled=_vector_disabled, collection_name=_config.collection_name, + candidate_strategy=candidate_strategy, + fusion_mode=fusion_mode, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/mcp_server.py` around lines 1620 - 1630, The retry call to search_memories is missing propagation of the requested ranking parameters: ensure the second call includes the candidate_strategy and fusion_mode arguments (e.g. search_memories(..., candidate_strategy=candidate_strategy, fusion_mode=fusion_mode, ...)) so a request that started with vector or rrf stays consistent on retry; verify candidate_strategy and fusion_mode are in scope and passed through unchanged from the original invocation/context.mempalace/normalize.py (1)
546-552:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle all non-dict
tool_use.inputvalues, not just lists.Line 547 only normalizes list inputs. If
inputis any other non-dict (e.g., string/number), Line 551+ callsinp.get(...)and crashes.Proposed fix
name = block.get("name", "Unknown") inp = block.get("input", {}) - if isinstance(inp, list): + if not isinstance(inp, dict): inp = {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/normalize.py` around lines 546 - 552, The code at block.get("input", {}) only guards against lists but later calls inp.get(...) (e.g., in the Bash branch: cmd = inp.get("command", "")), which will crash for non-dict inputs; update the normalization so that after inp = block.get("input", {}) you replace inp with {} unless isinstance(inp, dict) (not just list), ensuring subsequent uses like inp.get(...) and other branches safely assume a dict.
🟠 Major comments (21)
tests/test_collision_scan.py-124-136 (1)
124-136:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t lock in “same metadata means safe” for duplicate IDs.
This test assumes duplicate
drawer_ids are harmless when(source_file, chunk_index)matches, but the scan never compares the document payload. A batch can still contain the same ID and metadata with different content, and that will collapse to one row at write time. This should either assert document equality as part of the contract or treat same-ID duplicates as collisions regardless.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_collision_scan.py` around lines 124 - 136, The test currently treats duplicate drawer_id entries with identical (source_file, chunk_index) as safe without verifying document payload; update the test for test_assert_no_collisions_passes_on_incoming_duplicate_with_same_metadata to either (a) include the document payload in each proposed tuple and assert that assert_no_collisions verifies payload equality before allowing duplicates, or (b) change the expectation to treat any duplicate drawer_id in the incoming batch as a collision regardless of metadata by asserting that assert_no_collisions raises/returns a collision for duplicate IDs; reference assert_no_collisions, the proposed list entries, and _MockCollection to implement the chosen fix.mempalace/migrate.py-549-583 (1)
549-583:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
topics_by_wingrewrites to the palace being migrated.
migrate_wing_names()is palace-scoped, buttopic_renamesis built from the global known-entities registry and then applied wholesale. On installs with multiple palaces, migrating one palace can rename registry keys for another before that palace’s drawers/closets are migrated, leaving the registry and stored metadata out of sync. Filter the registry rewrite to only the old→new wing pairs planned from this run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/migrate.py` around lines 549 - 583, migrate_wing_names() currently applies topic renames globally; restrict those to only the wing renames planned for this run by filtering topic_renames before applying them. After computing topic_renames = _plan_topics_by_wing_renames(), build the set of planned pairs from d_updates and c_updates (e.g., set(d_updates.items()) ∪ set(c_updates.items())) and replace topic_renames with only entries whose (old,new) pair is in that set, then call _apply_topics_by_wing_renames(filtered_topic_renames) instead of applying the unfiltered topic_renames..github/workflows/publish.yml-76-76 (1)
76-76:⚠️ Potential issue | 🟠 MajorPin GitHub Actions to immutable commit SHAs (and disable checkout credential persistence)
.github/workflows/publish.ymluses movable refs (actions/checkout@v6at line 76,actions/setup-python@v6at line 114, andpypa/gh-action-pypi-publish@release/v1at line 124). Pin eachuses:entry to a full commit SHA to prevent upstream retags from changing executed code.Also,
actions/checkoutdefaultspersist-credentials: true(not overridden here). Setpersist-credentials: falseunless the workflow needs authenticated Git operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yml at line 76, Replace movable refs with immutable commit SHAs for all uses entries mentioned (actions/checkout@v6, actions/setup-python@v6, pypa/gh-action-pypi-publish@release/v1) by updating each `uses:` to the corresponding full commit SHA for that action, and for the actions/checkout step (the one currently using actions/checkout@v6) add `with: persist-credentials: false` to explicitly disable credential persistence unless the workflow needs authenticated git operations; ensure the changes target the checkout, setup-python, and pypa publish steps by updating their `uses:` tokens to SHAs and adding the persist-credentials setting to the checkout step.Source: Linters/SAST tools
mempalace/backends/qdrant.py-240-245 (1)
240-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently erase metadata on serialization failure.
If any metadata value is not JSON-serializable,
_jsonable_metadata()returns{}and the upsert still succeeds. That drops every metadata field for the point with no signal to the caller, which will break later filters and provenance reads.Suggested patch
def _jsonable_metadata(meta: dict | None) -> dict: - try: - value = json.loads(json.dumps(meta or {}, ensure_ascii=False)) - except (TypeError, ValueError): - value = {} - return value if isinstance(value, dict) else {} + if meta is None: + return {} + value = json.loads(json.dumps(meta, ensure_ascii=False)) + if not isinstance(value, dict): + raise ValueError("metadata must serialize to a JSON object") + return value🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/qdrant.py` around lines 240 - 245, The helper _jsonable_metadata currently swallows serialization failures and returns an empty dict; change it to preserve JSON-serializable fields and surface errors: in _jsonable_metadata first try to json.dumps(meta) as now; if that fails, iterate meta.items(), attempt to json.dumps each value individually, build a dict of the successfully-serializable entries, and if any keys were dropped raise a TypeError (or ValueError) listing the non-serializable keys so the upsert caller fails rather than silently losing metadata; return the filtered dict only when no keys were dropped, otherwise raise with a clear message.mempalace/backends/qdrant.py-1373-1383 (1)
1373-1383:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the palace marker before deleting remote collections.
Every open/read path uses the local marker to catch URL/namespace drift, but
delete_collection()rebuilds the target from the current config and deletes it directly. If the operator changedMEMPALACE_QDRANT_URLor the namespace since the palace was created, this path can delete the wrong remote collection.Suggested patch
def delete_collection(self, palace_path: str, collection_name: str) -> None: palace = PalaceRef(id=palace_path, local_path=palace_path) config = _QdrantConfig.from_options() + if os.path.isfile(self._marker_path(palace.local_path)): + self._validate_marker_target(palace, config) remote_collection = self._remote_collection_name( palace=palace, collection_name=collection_name, config=config, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/qdrant.py` around lines 1373 - 1383, The delete_collection method currently builds and deletes a remote collection from the live config without checking the palace's stored marker; to fix, load and validate the palace marker from the local palace (use PalaceRef/local marker loader) before computing _remote_collection_name: read the stored marker from PalaceRef(id=palace_path, local_path=palace_path), compare its Qdrant URL/namespace (or equivalent marker fields) to the current _QdrantConfig.from_options() values, and abort (raise/log and return) if they differ; only call _remote_collection_name, client.collection_exists and client.delete_collection after the marker matches to prevent deleting collections from a different remote/namespace.mempalace/config.py-956-962 (1)
956-962:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize backend aliases before validating/persisting.
backend_overrideaccepts aliases through_normalize_backend_name(), butset_backend()validates the raw lowercased string. Calls likeset_backend("chromadb"),set_backend("pg"), orset_backend("postgresql")now fail even though the rest of the config layer accepts them.Suggested patch
def set_backend(self, backend: str) -> None: """Persist the storage backend choice to ``config.json``.""" - backend = str(backend).strip().lower() + backend = _normalize_backend_name(backend) from .backends import get_backend_class🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/config.py` around lines 956 - 962, The set_backend method currently lowercases the input but doesn't apply existing alias normalization, causing valid aliases to be rejected; update set_backend to call the project's _normalize_backend_name (or equivalent) on the incoming backend string before calling get_backend_class and before writing to self._file_config["backend"] so validation and persistence use the normalized canonical backend name.hooks/antigravity/lib/common.sh-240-263 (1)
240-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't strip spaces out of real filesystem paths.
safe()removes spaces beforetranscriptPath,workspacePaths[0], andartifactDirectoryPathare returned, so a valid path like/Users/alice/My Project/chat.jsonlis rewritten to a different, nonexistent file. That will silently break mining on a common macOS path shape.Suggested patch
-def safe(s, allowed=r'[^a-zA-Z0-9_/.\-~]'): +def safe_path(s): + return re.sub(r'[^ a-zA-Z0-9_/.\-~]', '', str(s)) @@ -transcript = safe(data.get('transcriptPath', '')) +transcript = safe_path(data.get('transcriptPath', '')) @@ - workspace = safe(wp_arr[0]) + workspace = safe_path(wp_arr[0]) @@ -artifact = safe(data.get('artifactDirectoryPath', '')) +artifact = safe_path(data.get('artifactDirectoryPath', ''))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/antigravity/lib/common.sh` around lines 240 - 263, The current safe() function strips spaces (regex r'[^a-zA-Z0-9_/.\-~]') which mangles legitimate filesystem paths used when building transcript, workspace and artifact (variables transcript, workspace, artifact); update the sanitization so path-preserving characters (including space) are allowed—either relax safe()'s regex to include space or implement a dedicated safe_path() used for transcriptPath/workspacePaths[0]/artifactDirectoryPath that only removes truly dangerous characters while preserving spaces and normal path chars (refer to safe(), transcript, workspace, artifact in the diff).mempalace/searcher.py-629-632 (1)
629-632:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
search()still rejects valid non-Chroma palaces before this backend-aware path can run.The new metric probe is backend-aware, but the CLI still bails out earlier if
chroma.sqlite3is missing. A validsqlite_exactpalace only hassqlite_exact.sqlite3, somempalace searchwill incorrectly report “has no chroma.sqlite3 yet” even though the backend can open it. Please move the preflight behind backend detection/opening, or make the filesystem check backend-specific.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/searcher.py` around lines 629 - 632, The CLI currently performs a hard filesystem preflight that looks specifically for chroma.sqlite3 before backend detection, causing valid non-Chroma palaces (e.g., sqlite_exact.sqlite3) to be rejected; update the search() flow to defer that preflight until after backend detection/opening by calling get_collection or _metric_for_collection first (using palace_path) and only then perform backend-specific file checks, or alternatively make the preflight file check conditional on the detected backend (e.g., if backend=='chroma' check for chroma.sqlite3, if backend== 'sqlite_exact' check for sqlite_exact.sqlite3); modify code in search() to rely on get_collection/_metric_for_collection to identify the backend and then validate the corresponding file name instead of always requiring chroma.sqlite3.mempalace/backends/sqlite_exact.py-839-851 (1)
839-851:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the handle cache atomic.
Two threads opening the same
palace_pathcan both miss_clients, create separate SQLite connections, and then overwrite each other in the cache. That leaks one connection and, more importantly, splits in-process access across differentRLockinstances, so the serialization guarantee for SQLite writes disappears anddatabase is lockedfailures become much more likely.Suggested fix
with self._clients_lock: cached = self._clients.get(palace_path) if cached is not None and not cached.closed: return cached conn = sqlite3.connect(db_path, check_same_thread=False) conn.row_factory = sqlite3.Row lock = threading.RLock() handle = _SQLiteExactHandle(conn, lock) with handle.lock: self._init_schema(conn) with self._clients_lock: + cached = self._clients.get(palace_path) + if cached is not None and not cached.closed: + conn.close() + return cached self._clients[palace_path] = handle return handle🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/sqlite_exact.py` around lines 839 - 851, Two threads can race and create separate _SQLiteExactHandle instances for the same palace_path; fix by using a double-checked insert under _clients_lock: keep the initial quick check of self._clients under self._clients_lock, if missing create the sqlite3 connection and handle, then re-acquire self._clients_lock and check self._clients[palace_path] again—if another handle was inserted, close the newly-created conn and use the cached handle; otherwise store the new handle in self._clients; then perform schema initialization under handle.lock by calling _init_schema(conn). This uses the existing symbols _clients, _clients_lock, _SQLiteExactHandle, _init_schema, palace_path, conn, lock, and handle to ensure the cache insert is atomic and avoids leaked connections or multiple locks per DB.mempalace/backends/sqlite_exact.py-1008-1027 (1)
1008-1027:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
delete_collection()leaves the old embedder identity behind.
set_embedder_identity()persistsembedder_model:{collection_name}inmeta, but collection deletion only removesdocumentsandcollections. Recreating the same collection name will inherit the stale model name, so a “fresh” collection can immediately report the wrong embedder contract.Suggested fix
try: handle.conn.execute( "DELETE FROM docs_fts WHERE collection_id = ?", (collection_id,), ) except sqlite3.OperationalError: pass + handle.conn.execute( + "DELETE FROM meta WHERE key = ?", + (f"embedder_model:{collection_name}",), + ) handle.conn.execute("DELETE FROM collections WHERE id = ?", (collection_id,)) handle.conn.commit()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/sqlite_exact.py` around lines 1008 - 1027, delete_collection currently removes documents and the collection row but leaves the embedder identity stored in the meta table (set by set_embedder_identity as key "embedder_model:{collection_name}"), causing a recreated collection to inherit stale embedder info; update delete_collection to also delete the meta entry for the collection's embedder (delete from meta where key = f"embedder_model:{collection_name}") before committing, and ensure any other meta keys tied to the collection (if present) are removed as well to fully clean up state.mempalace/searcher.py-1950-1958 (1)
1950-1958:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t pre-rank and slice before
fusion_moderuns.
_finalize_candidate_hits()now always does_hybrid_rank(... )[:n_results]before the caller applies_FUSION_RANKERS[fusion_mode]. That meansfusion_mode="rrf"never sees the full union/hybrid pool, and BM25/graph candidates beyond the convex top-N are discarded before the later fusion and cross-encoder stages can consider them.Suggested fix
def _finalize_candidate_hits( @@ ) -> tuple: try: _apply_candidate_strategy( @@ return [], { "error": "candidate_strategy='union' requires a backend with lexical_search support", "unsupported_capability": "supports_lexical_search", "hint": "Use candidate_strategy='vector' or select a backend that supports lexical search.", } - - hits = _hybrid_rank(hits, query, metric=_metric_for_collection(drawers_col))[:n_results] - for h in hits: - h.pop("_sort_key", None) - h.pop("_source_file_full", None) - h.pop("_chunk_index", None) return hits, NoneLet the caller apply the requested fusion mode, rerank, and final trimming once.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/searcher.py` around lines 1950 - 1958, _fix in _finalize_candidate_hits: don't pre-rank and slice the hybrid pool; call _hybrid_rank(hits, query, metric=_metric_for_collection(drawers_col)) without [:n_results], then pass the full ranked list into _FUSION_RANKERS[fusion_mode] (or equivalent fusion step) and only apply the final [:n_results] trim after fusion/reranking and cross-encoder stages so fusion_mode (e.g., "rrf") sees the full candidate set; update any logic in _finalize_candidate_hits that assumes pre-sliced results so callers still get a final trimmed list.mempalace/backends/chroma.py-1724-1726 (1)
1724-1726:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet sqlite failures fall back to the client scan instead of returning zero hits.
lexical_search()treats any non-Nonereturn from_lexical_search_via_sqlite()as authoritative. Right now sqlite open/read failures return[], so a transient lock or schema issue becomes an empty result set even though the slowerself.get(...)fallback right below could still serve results. Reserve[]for “query succeeded with no matches” and returnNoneon infrastructure failure.Also applies to: 1780-1802, 1898-1900
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/chroma.py` around lines 1724 - 1726, The sqlite-backed lexical search should signal infrastructure failures with None, not an empty list, so update _lexical_search_via_sqlite to catch sqlite/open/read errors and return None on failure while still returning [] for a successful query with zero matches; then ensure callers in lexical_search (the blocks that currently do: sqlite_hits = self._lexical_search_via_sqlite(...) if sqlite_hits is not None: return LexicalResult(hits=sqlite_hits)) rely on that None sentinel—apply the same change for the other sqlite call sites mentioned (the other lexical_search code paths at the regions you flagged) so transient sqlite errors fall back to the slower self.get(...) scan instead of producing empty results.mempalace/backends/chroma.py-1723-1725 (1)
1723-1725:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSupport tag predicates in
lexical_search().
_validate_where()accepts$contains_all/$contains_any, but this path never strips and post-filters them the wayquery()/get()do. As written, a lexical search with tag filters falls through to_compare_metadata()and raisesUnsupportedFilterErroreven though the backend advertises those operators elsewhere.Also applies to: 1926-1928
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/chroma.py` around lines 1723 - 1725, lexical_search() currently calls _validate_where() then _lexical_search_via_sqlite() but never strips or applies tag predicates ($contains_all / $contains_any), causing _compare_metadata() to raise UnsupportedFilterError; update lexical_search() (and the same logic at the other occurrence around 1926-1928) to extract and remove any $contains_all/$contains_any entries from the where dict before calling _lexical_search_via_sqlite(), then post-filter sqlite_hits using the same tag-predicate logic used by query()/get() (or delegate to the existing _compare_metadata() once the tag predicates are stripped), ensuring tag predicates are honored for lexical searches.mempalace/backends/base.py-226-236 (1)
226-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep dimension mismatches hard-failing even when
force_model_swap=True.
force_model_swapcurrently bypasses both name and width conflicts. That lets a caller re-record a collection built with one vector dimension as another even though the stored vectors are physically incompatible. The force path should only relax model-name mismatches after dimensions already match or are unknown.Suggested fix
- if force_model_swap: - return "known_mismatch" - if dim_conflict: raise DimensionMismatchError( f"collection was built with a {stored.dimension}-dim embedder " f"({stored.model_name!r}) but the current embedder is " f"{current.dimension}-dim ({current.model_name!r}); the stored " "vectors are incompatible. Re-embed the palace to switch models." ) + if force_model_swap: + return "known_mismatch" raise EmbedderIdentityMismatchError(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/base.py` around lines 226 - 236, The current logic lets force_model_swap bypass dimension conflicts; change the control flow in the code around force_model_swap and dim_conflict so that DimensionMismatchError is raised unconditionally when stored.dimension and current.dimension are incompatible (i.e., keep the check that raises DimensionMismatchError using stored.dimension/current.dimension first), and only allow force_model_swap to relax embedder name mismatches (EmbedderIdentityMismatchError) when dimensions match or stored.dimension is unknown—ensure the DimensionMismatchError is raised before any early return for force_model_swap and that EmbedderIdentityMismatchError handling still considers force_model_swap only after dimensions are validated.mempalace/backends/chroma.py-1266-1268 (1)
1266-1268:⚠️ Potential issue | 🟠 Major_COLLECTION_TYPE_MARKER migration can permanently skip later collections
In
mempalace/backends/chroma.py,_fix_missing_collection_type()returns early once the palace-wide marker exists, so it never re-scanscollectionsagain:marker = os.path.join(palace_path, _COLLECTION_TYPE_MARKER) if os.path.isfile(marker): returnThe marker is then written after the first run (
Path(marker).touch()at ~1303-1305). If any collections are created later by chromadb <= 1.5.8 (which persistscollections.config_json_str='{}'without_type), future opens under chromadb 1.5.9+ (which deserializes viaCollectionConfigurationInternal.from_jsonand expects_type) can fail withKeyError: '_type'on those newer rows. This means later collections can be missed indefinitely after the first successful migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/chroma.py` around lines 1266 - 1268, _fix_missing_collection_type currently returns early when a single palace-wide marker (marker = os.path.join(palace_path, _COLLECTION_TYPE_MARKER)) exists, which prevents rescanning collections created after that first run; change the logic so the function always iterates the collections and perform migration per-collection, skipping only collections that have a per-collection marker (e.g. derive a marker name that includes the collection id/name like f"{_COLLECTION_TYPE_MARKER}-{collection_name}" and check os.path.isfile for that), and replace the single Path(marker).touch() with writing the per-collection marker after successfully migrating that collection; keep the overall palace scan but use per-collection markers to avoid permanently skipping later collections.mempalace/cli.py-1483-1487 (1)
1483-1487:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExit non-zero when backend resolution fails.
This branch prints an error but returns normally, so
mempalace syncexits with status 0 even though it could not determine the backend. That will make wrappers/CI treat a failed sync preflight as success.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/cli.py` around lines 1483 - 1487, The try/except around resolve_backend_name(palace_path) should exit with a non-zero status when it fails: after printing the error to sys.stderr (inside the except for Exception as exc) call sys.exit(1) (or re-raise SystemExit) instead of returning so the process exits with failure; update the exception handler in the CLI entrypoint that calls resolve_backend_name to ensure mempalace sync returns a non-zero exit code on backend resolution failure.mempalace/cli.py-6439-6440 (1)
6439-6440:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the backend override to a single
main()invocation.
_apply_backend_arg(args)mutatesos.environ, but none of the early-return paths inmain()restore the previous values. After one in-process call likemain()with--backend pgvector, a latermain()call without--backendwill still inherit that explicit override and can read/write the wrong storage backend.Suggested fix
args = parser.parse_args() - _apply_backend_arg(args) + _prev_backend = os.environ.get("MEMPALACE_BACKEND") + _prev_explicit_backend = os.environ.get(_EXPLICIT_BACKEND_ENV) + _apply_backend_arg(args) + + try: + # existing post-parse handling / dispatch stays here + ... + dispatch[args.command](args) + finally: + if _prev_backend is None: + os.environ.pop("MEMPALACE_BACKEND", None) + else: + os.environ["MEMPALACE_BACKEND"] = _prev_backend + if _prev_explicit_backend is None: + os.environ.pop(_EXPLICIT_BACKEND_ENV, None) + else: + os.environ[_EXPLICIT_BACKEND_ENV] = _prev_explicit_backend - - dispatch[args.command](args)Also applies to: 6453-6550
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/cli.py` around lines 6439 - 6440, _main() calls parser.parse_args() then _apply_backend_arg(args), which mutates os.environ and is never restored so subsequent in-process calls inherit the override; change this so the backend override is scoped to a single main invocation by saving the previous environment value, applying the override only for the duration of the main execution, and restoring the original value on all exit paths (including early returns and exceptions). Wrap the logic that uses the override (the remainder of main after _apply_backend_arg) in a try/finally or implement a small context manager around _apply_backend_arg(args) that saves os.environ.get('MEMPALACE_BACKEND') and restores it in finally so calls to main() without --backend do not see a prior override.mempalace/mcp_server.py-2647-2650 (1)
2647-2650:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe case-preservation branch is inverted and reintroduces raw input.
Lines 2647-2664 do the opposite of the comment: case-only updates keep the new sanitized value, while real renames overwrite it with the original
wing/roomargument. That bypassessanitize_write_name()/sanitize_name()on actual metadata changes and can persist whitespace/casing variants that break later wing/room filters.💡 Suggested fix
new_meta = dict(old_meta) if wing is not None: wing_pass = sanitize_write_name(wing, "wing") if wing_pass["error"]: return {"success": False, "error": wing_pass["error"]} update_sanitize_flags.extend(wing_pass["flags"]) try: new_meta["wing"] = sanitize_name(wing_pass["cleaned"], "wing") except ValueError as e: return {"success": False, "error": str(e)} - # Preserve existing casing when the caller passes a case-only - # variant (LLM clients often "autocorrect" acronyms like ps5→PS5). - if wing.lower() != str(old_meta.get("wing") or "").lower(): - new_meta["wing"] = wing + # Preserve existing stored casing for case-only changes. + old_wing = str(old_meta.get("wing") or "") + if old_wing and new_meta["wing"].lower() == old_wing.lower(): + new_meta["wing"] = old_wing if room is not None: room = _config.resolve_room(room) room_pass = sanitize_write_name(room, "room") if room_pass["error"]: return {"success": False, "error": room_pass["error"]} update_sanitize_flags.extend(room_pass["flags"]) try: new_meta["room"] = sanitize_name(room_pass["cleaned"], "room") except ValueError as e: return {"success": False, "error": str(e)} - # Preserve existing casing when the caller passes a case-only - # variant (LLM clients often "autocorrect" acronyms like ps5→PS5). - if room.lower() != str(old_meta.get("room") or "").lower(): - new_meta["room"] = room + # Preserve existing stored casing for case-only changes. + old_room = str(old_meta.get("room") or "") + if old_room and new_meta["room"].lower() == old_room.lower(): + new_meta["room"] = old_roomAlso applies to: 2661-2664
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/mcp_server.py` around lines 2647 - 2650, The branch comparing wing/room casing is inverted and may store raw, unsanitized input; change the logic so that case-only updates DO NOT overwrite existing metadata and real renames DO store the sanitized name: when processing wing/room, compute a sanitized value via sanitize_write_name() (or sanitize_name() where appropriate) and compare the lowercased sanitized/new value to old_meta.get("wing"/"room") lowercased; if they differ only by case, leave new_meta unchanged to preserve existing casing, otherwise set new_meta["wing"] or new_meta["room"] to the sanitized value (not the raw wing/room arg). Ensure the same fix is applied to both the wing and room branches (references: old_meta, new_meta, wing, room, sanitize_write_name(), sanitize_name()).mempalace/mcp_server.py-3473-3539 (1)
3473-3539:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconnect never drops the cached Postgres backend object.
tool_reconnect()clears_collection_cache, but_get_collection_postgres()only constructs a fresh backend when_postgres_backend_cache is None. This same file already nulls that cache on connection failure to avoid reusing a dead pool, so leaving it intact here means a reconnect after a postgres outage can reopen against the same stale backend object instead of forcing a real reset.💡 Suggested fix
def tool_reconnect(): """Force the MCP server to drop cached ChromaDB + KnowledgeGraph state. @@ global \ _client_cache, \ _collection_cache, \ + _postgres_backend_cache, \ _collection_cache_backend, \ _collection_cache_palace, \ _collection_open_error, \ _palace_db_inode, \ _palace_db_mtime, \ @@ _client_cache = None _collection_cache = None + _postgres_backend_cache = None _collection_cache_backend = None _collection_cache_palace = None _collection_open_error = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/mcp_server.py` around lines 3473 - 3539, tool_reconnect() currently clears many caches but does not reset the Postgres backend cache, so a stale connection pool in _postgres_backend_cache can be reused; update the reconnect cleanup to set the module-level _postgres_backend_cache = None (same place you clear _client_cache/_collection_cache/etc.) so that _get_collection_postgres() will construct a fresh backend on next use and avoid reusing a dead Postgres backend object.mempalace/backends/_sidecar.py-67-70 (1)
67-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist sidecar atomically to avoid partial-write corruption.
Directly writing
pathat Line 67 risks truncating the sidecar on interruption/crash mid-write; that can lose all collection entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/_sidecar.py` around lines 67 - 70, The current write-to-path in the sidecar saver risks truncation; change the implementation that writes the sidecar (the block using path, json.dump(data, ...), and os.chmod(path, 0o600)) to perform an atomic write: write JSON to a temp file in the same directory (e.g., path + ".tmp" or use tempfile.NamedTemporaryFile with dir=os.path.dirname(path)), flush and fsync the file descriptor, close it, set the permissions on the temp file, then atomically replace the target with os.replace(temp_path, path); ensure errors clean up the temp file and preserve the existing sidecar on failure, and continue to catch OSError/NotImplementedError as before.mempalace/backends/_sidecar.py-39-42 (1)
39-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMalformed
dimensioncan still raise despite “robust to malformed sidecar” contract.Line 41 can throw
ValueError/TypeErrorfor malformed JSON ("dimension": "abc"), which breaks the documented degrade-to-Nonebehavior.Proposed fix
- return EmbedderIdentity( - model_name=str(entry["model_name"]), - dimension=int(entry.get("dimension") or 0), - ) + try: + dimension = int(entry.get("dimension") or 0) + except (TypeError, ValueError): + return None + return EmbedderIdentity( + model_name=str(entry["model_name"]), + dimension=dimension, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mempalace/backends/_sidecar.py` around lines 39 - 42, The code currently does int(entry.get("dimension") or 0) which will raise ValueError/TypeError for malformed values like "abc"; update the conversion to be defensive: read raw = entry.get("dimension"), if raw is None or raw == "" set dimension = None, otherwise attempt to cast to int inside a try/except (catch ValueError and TypeError) and set dimension = None on failure, then pass that variable to EmbedderIdentity (reference symbols: entry, EmbedderIdentity and the function that returns it in _sidecar.py).
🟡 Minor comments (6)
.github/workflows/publish.yml-76-82 (1)
76-82:⚠️ Potential issue | 🟡 MinorDisable persisted checkout credentials before building.
actions/checkoutmay leave the workflow token/credentials persisted in local git config; the subsequent build runs repo-controlled code. Addpersist-credentials: falseto reduce credential exposure (token is already limited topermissions.contents: read).Suggested change
- uses: actions/checkout@v6 with: # Fully-qualified refs/tags/ so an unqualified name can't resolve to a # same-named *branch* instead of the tag (checkout prefers branches). # steps.tag.outputs.tag is format-validated above (ref-injection guard). ref: refs/tags/${{ steps.tag.outputs.tag }} fetch-depth: 0 # full history for the ancestry check + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yml around lines 76 - 82, In the actions/checkout@v6 step where you set the 'with' inputs (the block containing ref: refs/tags/${{ steps.tag.outputs.tag }} and fetch-depth: 0), add persist-credentials: false to the 'with' map so the workflow token/credentials are not left persisted in the local git config before running the build; update the checkout step (actions/checkout@v6) to include that key alongside ref and fetch-depth.Source: Linters/SAST tools
hooks/antigravity/install.sh-72-93 (1)
72-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate option values before
shift 2.
--install-dirand--log-levelboth read${2:-}and then unconditionallyshift 2. When the value is missing,set -eturns that into an immediate shell failure instead of your normal usage/error path.Suggested patch
--install-dir) - INSTALL_DIR="${2:-}" + if [ $# -lt 2 ] || [ -z "${2:-}" ]; then + echo "ERROR: --install-dir requires a value" >&2 + print_usage >&2 + exit 2 + fi + INSTALL_DIR="$2" shift 2 ;; @@ --log-level) - LOG_LEVEL="${2:-info}" + if [ $# -lt 2 ] || [ -z "${2:-}" ]; then + echo "ERROR: --log-level requires a value" >&2 + print_usage >&2 + exit 2 + fi + LOG_LEVEL="$2" shift 2 ;;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/antigravity/install.sh` around lines 72 - 93, The parsing for --install-dir and --log-level currently assigns "${2:-}" and does an unconditional shift 2 which can trigger set -e when the value is missing; update the option handling in install.sh to validate that a non-empty value exists in "$2" before calling shift 2 (for both the case patterns handling INSTALL_DIR and LOG_LEVEL), and if the value is missing, call the existing usage/error path (or set an explicit error and exit) instead of shifting; ensure the --install-dir=* and --log-level=* branches remain unchanged and only the long-form two-argument branches reference INSTALL_DIR and LOG_LEVEL after validation.tests/test_mcp_server.py-1274-1277 (1)
1274-1277:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
PalaceRefstrictly in this contract test.
getattr(closed, "local_path", closed)lets a raw string still pass, so this no longer enforces the post-MemPalace#1679 API contract.Suggested test-tightening patch
- closed = stub.closed[0] - assert getattr(closed, "local_path", closed) == config.palace_path + closed = stub.closed[0] + assert hasattr(closed, "local_path"), "close_palace should receive PalaceRef" + assert closed.local_path == config.palace_path🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_mcp_server.py` around lines 1274 - 1277, The test currently allows a raw string because it uses getattr(closed, "local_path", closed); change this to assert the contract by verifying closed is a PalaceRef and that closed.local_path equals config.palace_path (e.g., assert isinstance(closed, PalaceRef) and assert closed.local_path == config.palace_path), so the post-#1679 API requirement is strictly enforced for stub.closed[0].tests/test_antigravity_plugin_manifest.py-149-151 (1)
149-151:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire an explicit
typefield here.This assertion currently passes when the key is missing, which weakens the manifest contract the test is supposed to pin. A template that silently drops
typewould still go green.Suggested fix
- assert handler.get("type", "command") == "command", ( + assert "type" in handler and handler["type"] == "command", ( f"{event}: only type=command is supported by Antigravity" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_antigravity_plugin_manifest.py` around lines 149 - 151, The test currently uses handler.get("type", "command") which hides missing keys; change the assertion to require an explicit "type" key and that its value equals "command" by asserting presence (e.g., "type" in handler) and then comparing handler["type"] == "command" (or two assertions) so missing keys fail the test; update the assertion referencing handler.get to instead reference handler["type"] (and/or an explicit membership check) in the test function/assertion that contains the current handler.get usage.docs/fork-changes.yaml-32-32 (1)
32-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep manifest
summaryunder the documented 100-char limit.Line 32 exceeds the schema’s one-line summary length target, which makes rendered changelog rows harder to scan.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/fork-changes.yaml` at line 32, The manifest 'summary' value in docs/fork-changes.yaml is longer than the documented 100-character one-line target; shorten this string (the summary key's value) to be under 100 characters and keep it on a single line by removing or abbreviating details such as the long commit hash, parentheses or extra clauses (e.g., drop the hash or trim "RFC-001 backend stack, diary checkpoints restored, 113 commits" to a concise phrase) so the summary remains descriptive but within the length limit.tests/test_hallways_pagination.py-27-30 (1)
27-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten the mock guard so any
whereusage fails this regression test.Line 28 currently permits
wherecalls whenlimitis present, so a future switch toget(where=..., limit=...)could slip through undetected.Proposed fix
- if where is not None and limit is None: + if where is not None: raise RuntimeError("Error executing plan: too many SQL variables")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_hallways_pagination.py` around lines 27 - 30, The mock _get function currently only raises when where is set and limit is None, allowing future calls like _get(where=..., limit=...) to bypass this guard; update the guard in _get to unconditionally fail on any use of the where parameter (e.g., change the condition to check if where is not None) so any call to _get(..., where=...) raises the RuntimeError and the regression test catches such usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e2d154ea-7a37-41f5-aff2-b7fcd80d50f9
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (141)
.antigravity-plugin/README.md.antigravity-plugin/hooks.json.tmpl.antigravity-plugin/mcp_config.json.antigravity-plugin/plugin.json.antigravity-plugin/rules/mempalace-recall.md.antigravity-plugin/skills/mempalace-recall/SKILL.md.antigravity-plugin/skills/mempalace/SKILL.md.claude-plugin/README.md.claude-plugin/marketplace.json.claude-plugin/plugin.json.codex-plugin/README.md.codex-plugin/plugin.json.dockerignore.github/workflows/docker-publish.yml.github/workflows/publish.ymlCHANGELOG.mdDockerfileDockerfile.gpuFORK_CHANGELOG.mdREADME.mddocker-compose.ymldocker-entrypoint.shdocs/RELEASING.mddocs/fork-changes.yamldocs/integrations/opencode.mddocs/recovery/wing-name-migration.mddocs/specs/auto-query-integration.mdexamples/antigravity/README.mdexamples/antigravity/hooks.jsonexamples/antigravity/mcp_config.jsonhooks/README.mdhooks/antigravity/INVESTIGATION.mdhooks/antigravity/README.mdhooks/antigravity/STDIN_SHAPE.mdhooks/antigravity/install.shhooks/antigravity/lib/common.shhooks/antigravity/mempal_save_hook_antigravity.shhooks/antigravity/mempal_wake_hook_antigravity.shintegrations/shared/recall-protocol.mdmempalace/README.mdmempalace/backends/__init__.pymempalace/backends/_sidecar.pymempalace/backends/base.pymempalace/backends/chroma.pymempalace/backends/embedding_wrapper.pymempalace/backends/pgvector.pymempalace/backends/qdrant.pymempalace/backends/registry.pymempalace/backends/sqlite_exact.pymempalace/backups.pymempalace/cli.pymempalace/closet_llm.pymempalace/collision_scan.pymempalace/config.pymempalace/convo_miner.pymempalace/dedup.pymempalace/embedding.pymempalace/format_miner.pymempalace/hallways.pymempalace/hooks_cli.pymempalace/ids.pymempalace/knowledge_graph.pymempalace/layers.pymempalace/mcp_server.pymempalace/migrate.pymempalace/miner.pymempalace/normalize.pymempalace/palace.pymempalace/repair.pymempalace/searcher.pymempalace/version.pypyproject.tomltests/_backend_conformance.pytests/conftest.pytests/test_antigravity_hooks_install.pytests/test_antigravity_hooks_shell.pytests/test_antigravity_plugin_manifest.pytests/test_backend_conformance.pytests/test_backends.pytests/test_backups.pytests/test_clean_lone_surrogates.pytests/test_cli.pytests/test_collision_scan.pytests/test_config.pytests/test_convo_miner.pytests/test_convo_miner_unit.pytests/test_dedup.pytests/test_distance_metric.pytests/test_embedder_identity.pytests/test_embedding_wrapper.pytests/test_embeddinggemma.pytests/test_format_miner.pytests/test_hallways.pytests/test_hallways_pagination.pytests/test_hooks_cli.pytests/test_hybrid_search.pytests/test_ids.pytests/test_maintenance_hooks.pytests/test_mcp_mine.pytests/test_mcp_server.pytests/test_migrate.pytests/test_migrate_wings.pytests/test_miner.pytests/test_palace.pytests/test_pgvector_backend.pytests/test_qdrant_backend.pytests/test_repair.pytests/test_searcher.pytests/test_sqlite_exact_backend.pywebsite/.vitepress/api-sidebar.jsonwebsite/.vitepress/config.mtswebsite/guide/antigravity.mdwebsite/guide/claude-code.mdwebsite/guide/configuration.mdwebsite/guide/mcp-integration.mdwebsite/guide/openclaw.mdwebsite/public/llms-full.txtwebsite/reference/mcp-tools.mdwebsite/reference/modules.mdwebsite/reference/python-api/backends/_sidecar.mdwebsite/reference/python-api/backends/base.mdwebsite/reference/python-api/backends/chroma.mdwebsite/reference/python-api/backends/embedding_wrapper.mdwebsite/reference/python-api/backends/pgvector.mdwebsite/reference/python-api/backends/qdrant.mdwebsite/reference/python-api/backends/registry.mdwebsite/reference/python-api/backends/sqlite_exact.mdwebsite/reference/python-api/backups.mdwebsite/reference/python-api/cli.mdwebsite/reference/python-api/collision_scan.mdwebsite/reference/python-api/config.mdwebsite/reference/python-api/dedup.mdwebsite/reference/python-api/embedding.mdwebsite/reference/python-api/hallways.mdwebsite/reference/python-api/ids.mdwebsite/reference/python-api/index.mdwebsite/reference/python-api/mcp_server.mdwebsite/reference/python-api/migrate.mdwebsite/reference/python-api/miner.mdwebsite/reference/python-api/palace.mdwebsite/reference/python-api/searcher.md
Comment on lines
+40
to
+53
| def _delimited_sha256(parts: tuple[object, ...], truncate: int) -> str: | ||
| """Hash parts joined by the unambiguous delimiter, truncate to N hex chars. | ||
|
|
||
| Internal helper. Call sites should use the named ``make_*`` wrappers | ||
| below so the per-site contract is documented in code, not derived | ||
| from caller arguments. | ||
|
|
||
| Each part is coerced to ``str`` before joining so the helper mirrors | ||
| the pre-v2 behavior of ``f"{a}{b}"`` for ``None`` and numeric inputs — | ||
| e.g. ``valid_from=None`` joins as the literal string ``"None"`` rather | ||
| than crashing. | ||
| """ | ||
| key = _DELIM.join(str(p) for p in parts).encode() | ||
| return hashlib.sha256(key).hexdigest()[:truncate] |
There was a problem hiding this comment.
_delimited_sha256() is still ambiguous when a field can contain |.
Joining parts with a raw delimiter only works if that delimiter is impossible in every input component. That is not true here: make_drawer_id_from_content() hashes arbitrary content, and POSIX source_file names can also contain |. For example, ("a", "b|c", "d") and ("a|b", "c", "d") produce the same hashed key. That reopens the silent-overwrite class this module is meant to eliminate.
Suggested direction
def _delimited_sha256(parts: tuple[object, ...], truncate: int) -> str:
- key = _DELIM.join(str(p) for p in parts).encode()
- return hashlib.sha256(key).hexdigest()[:truncate]
+ h = hashlib.sha256()
+ for part in parts:
+ data = str(part).encode("utf-8")
+ h.update(len(data).to_bytes(8, "big"))
+ h.update(b"\0")
+ h.update(data)
+ return h.hexdigest()[:truncate]This changes the on-disk ID recipe, so ID_RECIPE will need a new version as part of the fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mempalace/ids.py` around lines 40 - 53, The current _delimited_sha256(...) is
vulnerable because raw delimiter collisions are possible; change the
canonicalization to an unambiguous encoding per part (e.g., coerce each part to
str, then prefix each with its byte-length or escape it — e.g., produce a
sequence like "<len>:<part>" for every part) before joining and hashing in
_delimited_sha256, so inputs like ("a","b|c","d") and ("a|b","c","d") no longer
collide; update any callers such as make_drawer_id_from_content to continue
using _delimited_sha256 and bump the module's ID_RECIPE version constant to
reflect the on-disk ID format change.
Collaborator
Author
There was a problem hiding this comment.
Verified and agreed in principle — though it requires a | inside source_file/content, so it's a much rarer shape than the #80 class. ids.py here is byte-identical to upstream/develop; per this sync's policy of not diverging inside upstream-new modules, filed upstream as 1776 → MemPalace#1776 with the length-prefix (netstring) direction and the id_recipe-v3 migration note.
There was a problem hiding this comment.
@jphein — Understood, and thanks for the thorough triage. The "requires | in a source path or content" bar does make it a lower-probability shape than the positional-concat class. Tracking it upstream at MemPalace#1776 with the netstring (length-prefix) direction and an id_recipe-v3 migration note is exactly the right call — that way the fix lands with the recipe bump and migration path all in one coherent upstream commit rather than as a fork-local divergence.
If you'd like me to resolve this review comment now that it's tracked upstream, just say the word.
jphein
added a commit
to techempower-org/palace-daemon
that referenced
this pull request
Jun 11, 2026
* feat(hooks): restore diary checkpoints in the silent stop path The 2026-05-14 mine-only refactor left the diary empty, so the session-start greeting's diary_read context went stale. The detached child now writes the AUTO-SAVE checkpoint entry via the daemon's mempalace_diary_write (agent_name=harness, topic=checkpoint) before triggering the transcript mine — diary as the marker the greeting reads, mine as the verbatim source. Mirrors the same restoration landing in the mempalace fork's hooks_cli (techempower-org/mempalace#348). Requested by JP 2026-06-11 ('we can have diaries again'). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * review(gemini): unwrap diary_write response, drop whitelisted-out session_id - session_id rides only in the entry text; the daemon executor whitelists agent_name/entry/topic/wing and silently drops the rest - _post_mcp only fails on transport errors — unwrap with _extract_inner and require the tool-level success flag before logging 'saved' - failures now log the actual inner/transport error detail Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
17 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Merges 113 upstream commits (~40 PRs) — the first sync since v3.3.6 (2026-05-24). Two commits: the merge itself (conflict resolutions in 21 files) and a post-merge reconciliation (test/lint/docs alignment).
In from upstream: the RFC-001 pluggable-backend stack (MemPalace#1679 + MemPalace#1727 metric-aware similarity, MemPalace#1731/MemPalace#1734 embedder identity, MemPalace#1732 advisory-locked maintenance hooks — the contract our conformance PR MemPalace#1769 exercises), wing-normalize (MemPalace#1675/MemPalace#1702, opt-in
migrate-wings), delimiter-safe drawer ids (MemPalace#1666, new drawers only), the additive-miningfile_already_minedordering fix, miner robustness (MemPalace#1137/MemPalace#1100/MemPalace#1102/MemPalace#1622/MemPalace#1602), MCP mine + hallways tools (now 37 tools), antigravity harness support, v3.4.0.Migration safety (vetted against the 409K-drawer production palace): wing normalization only happens via the explicit
migrate-wingscommand; new drawer ids use the v2 delimiter recipe but existing drawers keep v1 ids and file-level dedup (source_file + mtime) keeps re-mines duplicate-free. One operational note: upstream bumpedNORMALIZE_VERSION, so the next sweep will re-mine sources to apply noise-stripping — intentional and idempotent, but it will cost familiar some CPU.Headline resolution decisions:
palace.get_collectionunified: upstream resolution (explicit/config/env/markers + mismatch protection) + embedder-identity enforcement +EmbeddingCollectionwrap, with the fork's postgres DSN options, KG write-through attach, and collection_name None/DEFAULT shim. RETIRED-marker refusal intact._distance_to_similaritythreaded through_hybrid_rank; union strategy now uses the RFC-001lexical_searchcapability path (postgres tsvector branch kept for production).MEMPALACE_BACKEND;init()no longer seeds a backend key.Testing
ruff check+ruff format --checkclean;scripts/check-docs.sh✦ docs clean (changelog/llms-full/python-api regenerated, all tool-count claims at 37)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements
--backendCLI flagDocumentation