Skip to content

merge: upstream/develop into fork main (27 commits, last sync 2026-05-22) - #113

Merged
jphein merged 28 commits into
mainfrom
merge/upstream-develop-2026-05-22
May 22, 2026
Merged

merge: upstream/develop into fork main (27 commits, last sync 2026-05-22)#113
jphein merged 28 commits into
mainfrom
merge/upstream-develop-2026-05-22

Conversation

@jphein

@jphein jphein commented May 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Periodic upstream sync — 27 commits from `MemPalace/mempalace:develop` since the previous sync via #105 earlier today.

Notable upstream changes pulled in

  • Two of JP's own upstream PRs MERGED: #1142 (docs-releasing) and #1494 (docs-recovery-runbook). Now visible in our `main`.
  • Tunnel fixes: #1467 (tunnel-file config-aware path resolution) and #1468 (create_tunnel validates source/target rooms exist) — both architectural correctness fixes in `palace_graph`.
  • Hooks shallow-path guard: #1585 plus its gemini-review follow-up (`749d433`) — `hooks_cli` parents[3] access now guarded against transcript paths that don't have 3 ancestor levels.
  • Security-warning visibility fix: #1257 — scam-callout dangling link folded back into the alert.
  • README install-instructions polish: #1237 pipx, #1395 Claude Code retention, #1436 codex discoverability.

Conflict resolutions (3 files)

  • `CHANGELOG.md` — fork's two `Unreleased` headers (AGE-integration plan + postgres-cutover) kept; upstream's `Unreleased` tunnel-fix block re-headed to `Unreleased — 2026-05-22 — Upstream tunnels fixes (cherry-picked from MemPalace/develop)`. Both narratives preserved chronologically.
  • `README.md` — fork's title block and three fork-specific sections (`## The thesis`, the `## What this fork ships` axis-organized inventory, and `## Sources`) kept. Upstream's pipx/pip install paragraphs and Claude-Code retention checklist note dropped from where they were colliding — they're already covered by the !IMPORTANT callout at top + the existing `## Quickstart` further down. Upstream's CAUTION scam-alert block kept and rewired to include both upstream and fork repo URLs as official sources.
  • `mempalace/convo_miner.py` — single comment-wording conflict; kept the fork's `(Upstream feat: configurable chunk_size, chunk_overlap, min_chunk_size MemPalace/mempalace#1024 review fix.)` annotation that references the upstream PR by number.

Test plan

  • `ruff check .` + `ruff format --check .` clean
  • `pytest tests/test_convo_miner.py tests/test_convo_miner_unit.py` — 62 passed
  • Full suite verification by CI

🤖 Generated with Claude Code

oussamalembarki and others added 28 commits April 28, 2026 18:54
Fulfills the "Optional: release-checklist addition" proposal at the
bottom of MemPalace#1093 (the v3.3.2 release defect where plugin.json referenced
a mempalace-mcp binary that pyproject.toml never declared, so fresh
`pip install` was broken for everyone until messelink's #340 was
re-cut as v3.3.3).

New file at docs/RELEASING.md (no existing doc at that path) with a
single pre-release grep:

    grep -rn mempalace-mcp pyproject.toml .claude-plugin .codex-plugin

The original MemPalace#1093 proposal specified `pyproject.toml
.claude-plugin/plugin.json` (2 files). This expands via -rn directory
recursion to also cover `.claude-plugin/.mcp.json` and
`.codex-plugin/plugin.json`, which reference `mempalace-mcp` by name
too — same class of regression through a different surface. Happy to
trim to the narrower 2-file form if preferred; one-line edit.

Shows the concrete expected output so a maintainer running this under
release pressure can eyeball "pass" without mental translation, and
points at #340 as the historical fix anchor so "investigate why the
entry is missing" has a diagnostic starting point rather than a dead
end.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes from Copilot's 2026-04-23 inline review:

1. Drop `-n` from the grep command. Hard-coded line numbers in the
   "Expected" block would drift as files evolve, making the
   checklist misleading. The check is about presence, not location —
   line numbers add noise without helping pass/fail.

2. Reword "`console_script` entry point declared in pyproject.toml"
   → "console script declared under `[project.scripts]` in
   pyproject.toml". PEP 621's `[project.scripts]` is the canonical
   name for this repo's config form; the old wording conflated it
   with setuptools' `console_scripts` entry-point group name.

Expected output block updated to match new grep (no colons before
line numbers).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The project-files mine path (miner.mine) has wrapped _mine_impl in
mine_palace_lock since MemPalace#1264 — a non-blocking flock that raises
MineAlreadyRunning so the second runner exits cleanly instead of
queueing as a waiter that drives parallel HNSW inserts. The convos
mine path (convo_miner.mine_convos) was missing the same guard.

In practice this meant any caller that spawned `mempalace mine
--mode convos` repeatedly against the same palace — most notably
the Stop-hook transcript ingest before the per-target PID slot
landed — could stack up arbitrarily many concurrent mines, each
holding a ChromaDB client open, each writing to the same HNSW
index. Recently observed: 28 stuck convos mines on one machine
consuming ~18 GB of RAM and contributing to a load spike.

Fix: refactor mine_convos into a thin wrapper that holds the
per-palace flock around _mine_convos_impl, mirroring miner.mine
exactly. Dry-run skips the lock since it never writes.

Tests: two cross-process tests in tests/test_convo_miner.py —
one asserts MineAlreadyRunning when a child process holds the
lock, one asserts dry-run is unaffected. Same spawn-context
pattern as test_palace_locks.py (fork-with-chromadb deadlocks
on Python 3.13).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ruption

Documents the recovery procedure for the chromadb index-metadata corruption
shape filed at chroma-core/chroma#6949 and reproduced by mempalace's
rebuild_index code path (MemPalace#1492).

Symptom: mempalace integrity gate quarantines a segment dir with
"labels present but dimensionality is missing or invalid (None)" at
startup, vector search drops to BM25-only fallback, recall gap appears.

Recovery: patch the dimensionality field back into the index metadata
file (the rest of the segment state is intact). ~90 seconds end-to-end
on a 183k-drawer palace; restored 99.97% of recall.

The "delete the metadata file entirely" workaround from chroma-core/chroma#6949
loses the id_to_label and label_to_id mappings; the patch approach
documented here preserves them.

Companion content:
- docs/recovery/index-metadata-recovery.md (this file)
- Related issues MemPalace#1492 (producer-side fix) and MemPalace#1493 (auto-recover
  proposal for the integrity gate)
- External: jphein/palace-daemon docs/recovery/chromadb-metadata-dict-patch.md
  has the same procedure from a palace-daemon HTTP operator's
  perspective, plus tests/test_chromadb_metadata_recovery.py with a
  regression test that builds a real palace + corrupts + recovers.
palace_graph._TUNNEL_FILE was a module-level constant initialised from
os.path.expanduser("~") + "/.mempalace/tunnels.json", ignoring the
MempalaceConfig.palace_path config (and MEMPALACE_PALACE_PATH env var)
that drawers, KG, and every other piece of palace state honour. Under
any setup where $HOME and the configured palace diverge — subagent
profiles, sandboxes, multi-tenant hosts, container mounts moving the
palace to /srv/ — drawers landed in the configured palace while
tunnels silently landed in a different file invisible to other
processes touching the same palace.

Replace the constant with _get_tunnel_file(config=None) deriving the
path from a new MempalaceConfig.tunnel_file property (sibling of
palace_path). Default install unchanged because default palace_path
is ~/.mempalace/palace whose sibling tunnels.json is the legacy path.

Add a _legacy_tunnel_file() helper and a one-line WARNING in
_load_tunnels for the case where the configured tunnel file is missing
but the pre-fix hardcoded path has one. No auto-migration — silently
merging tunnel state across two locations risks clobbering newer data.

fix(graph): validate explicit-tunnel endpoints exist (MemPalace#1468)

create_tunnel previously only validated that wing/room names were
non-empty strings; nothing queried chroma to confirm at least one
drawer carried matching {wing, room} metadata. Pointing an explicit
tunnel at a phantom room silently succeeded. Combined with MemPalace#1467's
read-bubble, an agent could create_tunnel → list_tunnels and have both
calls return its own bogus write, self-confirming a tunnel that didn't
exist in the shared palace.

create_tunnel now calls _check_room_exists(wing, room, col) for both
endpoints before persisting an explicit tunnel; zero rows raises
ValueError naming the endpoint. Three deliberate carve-outs:

- kind != "explicit" skips validation because topic tunnels use
  synthetic topic:<name> room ids that don't correspond to real rooms
- _get_collection returning None (palace not yet created, transient
  failure, tests without backend) skips validation rather than
  fail-closed — matches tolerance pattern used throughout palace_graph
- Query exceptions are logged and treated as 'can't verify, allow' so
  a flaky index doesn't block legitimate writes

Behaviour change: callers that previously created tunnels pointing at
empty rooms (scaffolding before mining) will now raise. File the
drawer first, then create the tunnel.

Tests:
- _use_tmp_tunnel_file helper now also neutralises _get_collection so
  existing tests don't accidentally trip the new validation path when
  test-order pollution leaves a real chroma backend bound
- test_closets.py::TestTunnels setup/teardown updated to monkeypatch
  resolver functions instead of the removed constant; also neutralises
  _get_collection for the same reason
- Three tests in test_miner.py exercising compute_topic_tunnels are
  unchanged in intent — they monkeypatch the new resolvers and pass
  without stubbing _get_collection because kind=topic skips validation
- New TestTunnelFileFollowsConfig and TestCreateTunnelEndpointValidation
  classes cover the regression surface for both fixes
…flicts

The current 'pip install mempalace' instruction either fails outright
on PEP 668-managed Pythons (Debian/Ubuntu, Homebrew) or upgrades
chromadb / numpy / grpcio / click in the user's global site-packages
and breaks unrelated tools (numba, litellm, tutor, opentelemetry, ...).

mempalace ships a CLI, so pipx (or 'uv tool install') is the right
default — it isolates the install and still puts 'mempalace' on PATH.
Plain pip is kept as the alternative for users who want 'import
mempalace' inside their own venv.

Refs #284
… call

Three changes addressing MemPalace#1469 CI red + Gemini perf review:

1. ruff format (0.4.x) on tests/test_closets.py and
   tests/test_palace_graph_tunnels.py — the lint job pins
   ruff>=0.4.0,<0.5 and was flagging format drift.

2. Replace %r with '%s' in legacy / corrupt tunnel-file warnings.
   On Windows %r escapes backslashes in repr, so
   test_load_tunnels_warns_on_orphaned_legacy_file's
   'str(legacy) in caplog.text' assertion was failing on
   test-windows even though the warning was firing.

3. Address gemini-code-assist review on MemPalace#1469: pass a single
   MempalaceConfig() through _get_tunnel_file / _load_tunnels /
   _save_tunnels per create_tunnel call instead of each helper
   re-instantiating its own (which re-reads mempalace.yaml from
   disk). Helpers keep their config=None defaults so external
   callers and existing tests are unaffected.
Resolves conflicts from 128-commit divergence:

- mempalace/convo_miner.py imports: kept both `mine_palace_lock` (this PR)
  and `prefetch_mined_set` (develop).
- mempalace/convo_miner.py docstring: kept this PR's lock-wrapping
  description, added a one-line pointer to the chunking-config section
  whose body now lives in `_mine_convos_impl`.
- mempalace/convo_miner.py body: develop placed `cfg_chunk_size` /
  `cfg_min_chunk_size` setup inline in `mine_convos`. This PR factored
  the body into `_mine_convos_impl`, so the inline setup would have
  left `cfg_chunk_size` referenced-but-undefined inside the impl.
  Moved the `MempalaceConfig()` setup into `_mine_convos_impl` so the
  variables are in scope where they're used.
- tests/test_convo_miner.py: kept both additive test sets (lock
  concurrency from this PR + wing_api auto-routing from develop).

Local: ruff check / format pass; full pytest suite passes
(2103 passed, 3 skipped).

Co-Authored-By: Igor Lins e Silva <4753812+igorls@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ency

fix(convo_miner): wrap mine_convos in mine_palace_lock
…ation

Resolves conflicts from 60-commit divergence:

- tests/test_closets.py: assertion reformat — kept develop's ruff-format-
  preferred multi-line shape (functionally identical).
- tests/test_palace_graph_tunnels.py: both branches added a new test
  class at end-of-file (this PR's TestTunnelFileFollowsConfig + develop's
  TestEntityTunnels from MemPalace#1564). Kept both, no overlap.

Local: ruff check / format pass; full pytest suite passes
(2113 passed, 3 skipped).

Co-Authored-By: Igor Lins e Silva <4753812+igorls@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…fig-and-endpoint-validation

fix(graph): tunnel file follows palace_path; validate explicit-tunnel endpoints exist
``_mempalace_python()`` in ``mempalace/hooks_cli.py`` uses
``Path(__file__).resolve().parents[3]`` to locate the venv Python
interpreter in the standard install layout
``<venv>/lib/pythonX.Y/site-packages/mempalace/hooks_cli.py``. When the
package lives at a shallow filesystem path — Docker containers
mounting at ``/work``, ``/opt/app``, minimal-prefix production
installs — ``parents`` has fewer than 4 elements and the index raises
``IndexError`` instead of falling through to the editable-install
branch.

The crash was caught by OrbStack-based triple-Python CI verification
on PR MemPalace#1579: 16 tests in ``test_hooks_cli.py`` failed identically on
Linux 3.9 / 3.11 / 3.13 with the same ``IndexError: 3`` from
``pathlib._PathBase.parents.__getitem__`` — and verified pre-existing
on develop tip in the same container. The bug never surfaces in
GitHub Actions CI runners (their workdir at
``/home/runner/work/mempalace/mempalace`` has plenty of parent
directories) but it surfaces immediately for anyone:

  - running mempalace in editable mode inside a Docker dev container
  - shipping mempalace as part of an OCI image where the install
    prefix is ``/app`` or ``/opt/<name>``
  - using OrbStack / Colima / podman-machine for cross-version
    verification

## The fix

Wrap each ``parents[N]`` access in ``try/except IndexError`` so the
helper falls through to the next strategy (editable-install →
``sys.executable``) instead of crashing the hook. Both ``parents[3]``
AND ``parents[1]`` are guarded — the latter is defensive against
extreme cases like a file at root (``/file.py``, parents=[/]) — same
class of bug.

## Test added (RED-first, then GREEN)

  tests/test_hooks_cli.py::test_mempalace_python_handles_shallow_path_without_crashing

Mocks ``Path(__file__).resolve()`` so ``parents[3]`` raises
``IndexError`` and ``parents[1]`` returns a real shallow path
(``/work/mempalace``). Pre-commit: function raises ``IndexError: 3``.
Post-commit: function returns a valid Python interpreter path
(either editable-venv if present, otherwise ``sys.executable``).

## Verification

  pytest tests/test_hooks_cli.py
    → 110 passed, 1 skipped on macOS (the existing run)
    → 110 passed, 1 skipped on Linux 3.9 / 3.11 / 3.13 (OrbStack)
       — was 16 failed, 94 passed before this commit

  ruff check + ruff format --check (pinned 0.15.9)
    → All checks passed; 2 files already formatted
…de_effect

Two medium-priority gemini-code-assist comments on PR MemPalace#1580 both
recommend more-idiomatic Python:

1. **Production code (``mempalace/hooks_cli.py::_mempalace_python``)** —
   replace ``try/except IndexError`` with ``if len(parents) > N:``
   look-before-you-leap checks. Exception handling for bounded-integer
   index lookups is a code smell in Python; LBYL makes the depth check
   explicit and removes exception overhead. Same behavior, clearer
   intent.

   Before (EAFP, ~12 lines + comment):
       try:
           venv_bin = resolved.parents[3] / "bin" / "python"
           if venv_bin.is_file():
               return str(venv_bin)
       except IndexError:
           pass

   After (LBYL, ~5 lines):
       if len(parents) > 3:
           venv_bin = parents[3] / "bin" / "python"
           if venv_bin.is_file():
               return str(venv_bin)

2. **Test code (``tests/test_hooks_cli.py``)** — replace the lambda +
   generator-throw hack with ``MagicMock.side_effect = get_item``,
   where ``get_item`` is a normal function that returns the
   editable-install path for index 1 and raises ``IndexError`` for
   any other index (defensive against a future regression that drops
   the LBYL length check). Standard ``side_effect`` mocking pattern.

   Before:
       fake_parents.__getitem__ = lambda self, idx: (
           RealPath("/work/mempalace")
           if idx == 1
           else (_ for _ in ()).throw(IndexError(idx))
       )

   After:
       def get_item(idx):
           if idx == 1:
               return RealPath("/work/mempalace")
           raise IndexError(idx)

       fake_parents.__len__.return_value = 3
       fake_parents.__getitem__.side_effect = get_item

   Also added ``__len__`` mock so the LBYL length check in production
   sees the simulated shallow path correctly.

## Verification

  pytest tests/test_hooks_cli.py
    → 110 passed, 1 skipped (same as PR MemPalace#1580 baseline; regression
       test for shallow-path crash still GREEN)

  ruff check + ruff format --check (pinned 0.15.9)
    → All checks passed; 2 files already formatted
docs(recovery): runbook for chromadb dimensionality=None metadata corruption
docs: add RELEASING.md with mempalace-mcp pre-release check
…ert callout

The original sentence ('may distribute malware. Details and timeline:
docs/HISTORY.md') was split mid-sentence by the visibility reformat,
leaving a fragment 'malware. Details and timeline: ...' as an orphaned
blockquote outside the [!CAUTION] callout. Fold the link into the
malware line so the callout stays self-contained.

Co-Authored-By: Igor Lins e Silva <4753812+igorls@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bring docs PR up to date with develop so CI re-runs against the
current pin set.

Co-Authored-By: Igor Lins e Silva <4753812+igorls@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bring docs PR up to date with develop so CI re-runs against the
current pin set.

Co-Authored-By: Igor Lins e Silva <4753812+igorls@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bring docs PR up to date with develop so CI re-runs against the
current pin set.

Co-Authored-By: Igor Lins e Silva <4753812+igorls@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…arning-visibility

docs: improve visibility of phishing/malware warnings
…tion-setup

docs: add Claude Code retention setup checklist
…ability

docs: make Codex MCP setup discoverable
…stall

docs(readme): recommend pipx for install (fixes PEP 668 + global dep conflicts)
…hallow-path-guard

fix(hooks_cli): guard parents[3] access against shallow filesystem paths
…evelop-2026-05-22

# Conflicts:
#	CHANGELOG.md
#	README.md
#	mempalace/convo_miner.py
Copilot AI review requested due to automatic review settings May 22, 2026 16:13
@gemini-code-assist

Copy link
Copy Markdown

Summary of Changes

Hello, 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 integrates a significant set of upstream changes into the fork, bringing various bug fixes, architectural improvements, and new documentation. The primary focus is on enhancing the stability and correctness of the tunneling system, improving the robustness of CLI hooks in diverse environments, and providing clearer guidance for users, particularly regarding Claude Code integration and data recovery. The merge also includes updates to the changelog and README to reflect these changes and address security advisories.

Highlights

  • Upstream Sync & Conflict Resolution: Synchronized the fork with 27 commits from the upstream MemPalace/mempalace:develop branch, resolving conflicts in CHANGELOG.md, README.md, and mempalace/convo_miner.py to preserve both fork-specific and upstream changes.
  • Tunneling System Improvements: Implemented fixes for explicit tunnels, ensuring their storage path respects MempalaceConfig.palace_path and adding validation to create_tunnel to confirm source and target rooms exist in the Chroma index, preventing tunnels to phantom rooms.
  • Hooks Shallow-Path Guard: Added a guard to hooks_cli to prevent IndexError when accessing parents in shallow filesystem paths, improving robustness in environments like Docker containers.
  • Documentation & User Guidance: Introduced new documentation for releasing procedures and a comprehensive recovery guide for chromadb segment corruption. Updated README.md with a critical security warning and added a dedicated guide for Claude Code retention.
  • MCP Integration Enhancements: Expanded MCP setup guidance in the CLI and documentation to explicitly include Codex CLI, alongside Claude Code, for broader tool integration.
  • Concurrency Control for Convo Mining: Refactored mine_convos to use a per-palace lock, preventing concurrent mining operations from conflicting, while allowing dry-runs to bypass the lock.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@jphein
jphein merged commit 65c781b into main May 22, 2026
10 of 11 checks passed
@jphein
jphein deleted the merge/upstream-develop-2026-05-22 branch May 22, 2026 16:16

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several critical fixes and enhancements to the MemPalace system, primarily focusing on tunnel management and session retention. Key changes include relocating the tunnels.json file to follow the configured palace_path (fixing a bug where tunnels were stored in a hardcoded home directory), adding validation to ensure explicit tunnel endpoints exist in the Chroma index, and implementing a per-palace lock for conversation mining to prevent concurrent write issues. Additionally, the PR adds extensive documentation for Claude Code session retention and recovery procedures for corrupted ChromaDB metadata. Feedback highlights an opportunity to optimize endpoint validation by combining database queries and suggests a more defensive approach to handling corrupted tunnel files to prevent accidental data loss during overwrites.

Comment thread mempalace/hooks_cli.py
Comment on lines +87 to +90
if len(parents) > 3:
venv_bin = parents[3] / "bin" / "python"
if venv_bin.is_file():
return str(venv_bin)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The logic to find the venv root by indexing parents[3] assumes a specific directory depth (e.g., <venv>/lib/pythonX.Y/site-packages/mempalace/). While the added len(parents) > 3 guard prevents crashes on shallow paths, the hardcoded index 3 might still point to the wrong directory (like .../lib/) in some Python environments. Consider using a more robust way to find the bin directory relative to the package root, or verify if parents[4] is more appropriate for standard lib/pythonX.Y/site-packages layouts.

Comment thread mempalace/palace_graph.py
Comment on lines +507 to +511
logger.warning(
"Mempalace tunnels file '%s' is corrupt or unreadable; starting empty.",
current_tunnel_file,
)
return []

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

In _load_tunnels, if the JSON file is corrupt, the function logs a warning and returns an empty list. In the context of create_tunnel, this list is then mutated and saved, which will effectively overwrite the corrupt file and cause data loss of all previous tunnels. While this matches the previous silent behavior, a more defensive approach would be to raise an error or move the corrupt file to a backup location before starting fresh.

Comment thread mempalace/palace_graph.py
Comment on lines +682 to +685
if not _check_room_exists(source_wing, source_room, col):
raise ValueError(f"Source room '{source_room}' does not exist in wing '{source_wing}'")
if not _check_room_exists(target_wing, target_room, col):
raise ValueError(f"Target room '{target_room}' does not exist in wing '{target_wing}'")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Validating room existence for explicit tunnels is a great correctness improvement. However, calling _check_room_exists twice (once for source, once for target) results in two separate queries to ChromaDB. While acceptable for low-frequency operations, these could be combined into a single query using a $or filter on the metadata to reduce overhead.

Suggested change
if not _check_room_exists(source_wing, source_room, col):
raise ValueError(f"Source room '{source_room}' does not exist in wing '{source_wing}'")
if not _check_room_exists(target_wing, target_room, col):
raise ValueError(f"Target room '{target_room}' does not exist in wing '{target_wing}'")
if kind == "explicit":
col = _get_collection(config)
# Check both rooms in a single query for efficiency
if col is not None:
try:
query = {"$or": [
{"$and": [{"wing": source_wing}, {"room": source_room}]},
{"$and": [{"wing": target_wing}, {"room": target_room}]}
]}
results = col.get(where=query, limit=2, include=[])
found = {(r["wing"], r["room"]) for r in results.get("metadatas", [])}
if (source_wing, source_room) not in found and not _check_room_exists(source_wing, source_room, col):
raise ValueError(f"Source room '{source_room}' does not exist in wing '{source_wing}'")
if (target_wing, target_room) not in found and not _check_room_exists(target_wing, target_room, col):
raise ValueError(f"Target room '{target_room}' does not exist in wing '{target_wing}'")
except Exception:
pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants