Skip to content

merge: upstream/develop into fork main (57 commits, last sync 2026-05-21) - #105

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

merge: upstream/develop into fork main (57 commits, last sync 2026-05-21)#105
jphein merged 60 commits into
mainfrom
merge/upstream-develop-2026-05-21

Conversation

@jphein

@jphein jphein commented May 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Periodic upstream sync — 57 commits from MemPalace/mempalace:develop since the last sync on 2026-05-13. Same pattern as the previous merge (6058489).

Notable upstream changes pulled in:

Conflict resolutions

7 files had conflicts. Resolution log lives in the merge commit message.

  • .gitignore — kept both (fork's benchmarks/c_beta rules + upstream's .review-logs)
  • pyproject.toml — took upstream's ruff==0.15.9, kept fork's postgres extras
  • mempalace/cli.py--mode choices is now ["projects", "convos", "session", "extract"] (union of fork's session and upstream's extract); stacked fork's --workers + upstream's --max-chunks-per-file
  • mempalace/convo_miner.py — kept fork's mine_sessions alongside upstream's new helpers
  • mempalace/mcp_server.py — merged fork's room-taxonomy warnings with upstream's chunked-write idempotency; preserved session_id metadata; daemon-strict path now also wires _start_idle_exit_watchdog for the local branch only
  • mempalace/multi_encoder.py — kept the in-flight sha256 _doc_key fix (Gemini PR feat(age-integration): 6-phase mempalace knowledge base ↔ AGE integration #101 medium)
  • tests/test_hooks_cli.py + tests/test_miner.py — union of fork's daemon-routed test suite + power-resilience tests + upstream's _claim_mine_slot and prefetch_mined_set tests

Also: ruff 0.15.9 reformatted 10 fork-only files; added from typing import Optional to mempalace/miner.py for Optional[int] parameters added by MemPalace#1455.

Test plan

  • pytest tests/ -k 'age or kg or knowledge_graph or multi_encoder or writethrough or pending_queue or backend_unreachable or miner_unit or hooks_cli' — 397 passed, 14 skipped
  • All conflict markers verified removed
  • ruff check . clean
  • ruff format --check . clean
  • Full CI sweep (this PR triggers it)

🤖 Generated with Claude Code

milla-jovovich and others added 30 commits April 27, 2026 01:57
When mempalace mine --mode convos is invoked against a directory inside
a known AI-tool storage path (Claude Code, Codex CLI, Gemini CLI), the
destination wing now auto-defaults to wing_api rather than the directory
basename. Conversations from external API-keyed tools land grouped under
a single dedicated wing for visibility.

Detected paths (exact-segment match — substrings like .gemini-backup or
.codex-archive do NOT match):

  - any segment .codex (Codex CLI sessions / archives)
  - any segment .gemini (Gemini CLI sessions under ~/.gemini/tmp/...)
  - the consecutive segment pair .claude/projects (Claude Code).
    .claude alone is NOT matched - that is the settings/config dir,
    not a conversation source.

Wing-resolution precedence (first match wins):

  1. Explicit --wing argument from the user - always wins
  2. AI-tool path detection -> wing_api
  3. Basename fallback (existing behavior, unchanged)

Two new helpers split out of mine_convos for unit-test coverage:

  - _is_ai_tool_path(path: Path) -> bool
  - _resolve_wing(convo_path: Path, wing: Optional[str]) -> str

mine_convos now calls _resolve_wing in place of its inline basename
logic. No other call sites or downstream consumers change.

Test coverage:

  - 15 unit tests covering positive matches (Claude Code subdir + root,
    Codex root + sessions, Gemini root + chats), negative cases
    (.claude alone is settings dir, unrelated paths, substring no-match
    on .gemini-backup / .codex-archive), explicit --wing override,
    auto-route trio, basename fallback, empty-string-as-no-wing.
  - End-to-end smoke test (manual): real-shape Claude Code JSONL fixture
    mined via the actual CLI; sqlite read-back of /tmp palace confirms
    drawers landed with wing='wing_api' and verbatim content preserved;
    mempalace search --wing wing_api returns expected content ranked.
  - Full pytest sweep: 1388 baseline + 15 new = 1403 passed, zero
    regressions.

Design context:

This change reflects Aya's product call that conversations from
API-keyed AI tools should land in a structural wing_api rather than be
scattered across topical wings derived from directory basenames. Igor's
ADR-0017 in mempalace-ts proposes the alternative of source-prefix
metadata (source LIKE 'api/%') with topical wing assignment instead;
that approach has architectural merit (wings stay topical) but does not
deliver the single-wing visibility users get here. Open for review
discussion - explicit --wing flag and basename fallback both unchanged,
so this is additive and reversible.

Closes part of #59 for the auto-routing UX.
Two resource leaks in mempalace.migrate that bite hardest on Windows:

1. extract_drawers_from_sqlite() opened a sqlite3 connection at the
   top and only closed it in the happy path. Any exception during
   query/iteration leaked the handle, leaving a file lock on
   chroma.sqlite3 that prevented the rest of the migration from
   touching the palace directory. Wrap with contextlib.closing().

2. migrate() called tempfile.mkdtemp() and never cleaned it up if a
   later step (chromadb open, batch import, count, swap) raised.
   The orphaned palace under the system temp root could be 100s of
   MB and never gets reclaimed. Wrap the import-and-swap dance in
   try/finally and rmtree the temp dir if it still exists at the
   end (os.replace consumes it on the happy path so the existence
   guard makes that case a no-op).

Tests cover the happy-path extraction and verify mkdtemp's directory
is removed when ChromaBackend.get_or_create_collection() raises.
…mPalace#1505)

Mining a transcript with --extract general was silently skipped when the
same file had already been mined with --extract exchange (or vice versa)
because file_already_mined() and prefetch_mined_set() only looked at
source_file. The two extraction modes produce different drawer content
and rooms, so they should coexist for the same source.

Changes:
- file_already_mined() and prefetch_mined_set() take an optional
  extract_mode arg and only return True when stored drawer metadata
  matches. Legacy drawers without extract_mode are treated as
  exchange-mode for back-compat.
- _file_chunks_locked() purges only same-mode drawers when rebuilding
  on a normalize-version bump, so a schema bump on one mode does not
  drop drawers filed under the other.
- Drawer ids and sentinel ids include extract_mode so the two modes
  cannot collide on hash.
- Pagination on the direct skip-check path so large transcripts (>1k
  drawers) are classified correctly when the bulk prefetch is skipped.

Adds regression coverage for the extract-mode-aware helper, the
pagination path, and an end-to-end mine_convos run that files
exchange then general for the same transcript without skipping.
Address PR MemPalace#1528 review feedback (gemini-code-assist, medium):
_source_file_delete_ids duplicated the legacy-drawer matching logic
that already lives in palace._metadata_matches_extract_mode.

Import the helper and call it instead so the back-compat rule
(legacy drawers without extract_mode count as exchange-mode) has a
single source of truth.

No behavior change. All 71 miner/convo tests still pass.
…ks (MemPalace#1438)

The PR reformatted two pre-existing assertions with a newer ruff
(0.5+ `assert X, (\n msg\n)` style) that CI's pinned ruff
(>=0.4.0,<0.5) rejects, failing `ruff format --check .`. Revert those
two unrelated blocks to develop's 0.4.x form; the genuine MemPalace#1435 fix and
its new regression tests are untouched.
Fix UTF-8 lock holder writes on Windows
…1528)

CI pins ruff>=0.4.0,<0.5 (resolves 0.4.10); one new assert block was
laid out in ruff-0.5+ style, failing `ruff format --check .`.
Reformat with the exact CI ruff version (assertion layout only, no
semantic change). `ruff check .` + `ruff format --check .` both pass
under 0.4.10.
…ode-aware-skip

fix(convo_miner): scope skip-check and drawer ids by extract_mode (MemPalace#1505)
CI installed `ruff>=0.4.0,<0.5` (resolves 0.4.10) while contributors
run modern ruff (0.5+). ruff 0.5 changed assert-message formatting, so
valid code formatted by a current ruff fails CI's `ruff format --check .`
on layout alone. This blocked MemPalace#1438, MemPalace#1445, and MemPalace#1528 — each needing a
manual reformat-and-push cycle with no actual code defect.

Pin an exact, modern ruff in both pyproject dev lists and in ci.yml so
CI and `pip install -e ".[dev]"` format/lint identically. No new
`ruff check` violations under the existing E/F/W/C901 select.
One-time mechanical reformat so `ruff format --check .` passes under the
newly pinned ruff. Layout only (assert-message parenthesization etc.),
no behavior change. 29 files: 28 under tests/ + 1 tools helper, no core
mempalace/ modules. Produced by `ruff format .`.
Addresses Copilot review: the ruff pin was incomplete.
- .pre-commit-config.yaml stayed on ruff-pre-commit v0.4.10, so
  contributors running pre-commit would reintroduce the exact 0.4-vs-0.5
  formatter drift this PR removes. Bumped rev to v0.15.9 and rewrote the
  lock-step comment to point at pyproject as the source of truth.
- uv.lock still constrained dev ruff at >=0.4.0, so `uv sync --extra dev`
  (the documented setup) would not honor the new exact pin. Regenerated;
  the dev specifier is now ==0.15.9 in both lock entries.

ruff check/format still pass; no formatting delta from these files.
ci(lint): pin ruff to 0.15.9 (CI + dev) and reformat tree
…1534)

`_chunk_by_paragraph` and its 25-line-group fallback appended whole
paragraphs and line groups as single drawers with no per-drawer size
cap, so a normalized transcript containing a single >CHUNK_SIZE
paragraph (for example a Claude Code session with pasted content as
one 135,600-char line) produced one giant chunk. ChromaDB upsert then
fed that chunk to the embedding model, blowing the O(seq_len^2)
attention budget with `RuntimeError: Invalid buffer size: 120.00 GiB`.

Introduce a single `_emit_bounded` helper that gates the whole stripped
content against `min_chunk_size` (noise filter) and emits every slice
verbatim via an index-based `range(0, len, chunk_size)` loop. The
helper preserves trailing remainders instead of silently dropping them
and avoids the O(N^2) substring-copy pattern. Both `_chunk_by_paragraph`
and `_chunk_by_exchange` now delegate slicing to the helper. Plumb the
config-resolved `chunk_size` through `_chunk_by_paragraph` so
`MempalaceConfig.chunk_size` governs this path too.

Co-Authored-By: David F Glidden <4116848+davidglidden@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Repeated `repair --yes` runs leave freed SQLite pages unreclaimed and
can corrupt the FTS5 inverted index. This patch adds
`_vacuum_and_rebuild_fts5()`, called at the end of `rebuild_index()`,
which:

1. Closes all chroma handles (releases PersistentClient's sqlite lock).
2. Rebuilds the FTS5 index via the built-in content-table DDL trick.
3. Runs VACUUM (requires autocommit / isolation_level=None) to reclaim
   the freed pages.

The function is a no-op when chroma.sqlite3 is absent or the FTS5
table doesn't exist, so it is safe on every supported backend.

Four new tests cover the happy path, missing FTS5 table, missing file,
and call ordering (close must precede vacuum).

Closes MemPalace#1516
Closes MemPalace#1517
…m-quarantine

fix: avoid quarantining recoverable HNSW metadata
…uum-fts5

fix: VACUUM + FTS5 rebuild after repair to reclaim SQLite space
…rce-cleanup

fix(migrate): close SQLite connection and clean temp palace on exception
…Palace#1473)

tool_create_tunnel sanitized names inside a try/except ValueError but
called create_tunnel() *outside* it, so any ValueError create_tunnel
raises (empty/non-string endpoints today, room-existence checks once
MemPalace#1469 lands) escaped and the MCP framework wrapped it as the opaque
'Internal tool error'. Wrap the create_tunnel() call in the same
try/except and return {"error": str(e)}, mirroring sibling tools
(tool_add_drawer, tool_list_tunnels). Adds a regression test that
monkeypatches create_tunnel to raise and asserts the message is
surfaced verbatim.

Closes MemPalace#1473
…ace#1510)

Nine save/log/precompact tests in test_hooks_cli.py passed only because
test_cli.py (alphabetically earlier) created ~/.mempalace in the session
tmp HOME as a side effect, satisfying the _palace_root_exists()
kill-switch. Run in isolation they short-circuited and failed (9 failed,
80 passed, 1 skipped).

Add a module autouse fixture that points PALACE_ROOT/STATE_DIR at a
per-test palace root that exists, so every test is robust standalone and
future tests don't inherit the trap. Kill-switch tests that need the
absent path call _redirect_palace_root after the fixture; monkeypatch
last-write-wins keeps their absent/file root and teardown restores the
real module value.

Isolation: pytest tests/test_hooks_cli.py -> 100 passed, 1 skipped.
Ordering preserved: test_cli + test_hooks_cli -> 165 passed.

Closes MemPalace#1510
Review feedback: the sanitize_name and create_tunnel calls both raise
ValueError and returned the identical {"error": str(e)} shape, so the
second try/except was redundant nesting. Fold create_tunnel into the
single guard; behavior and the MemPalace#1473 regression test are unchanged.
…lace#1510)

Review feedback: _MINE_PID_DIR is derived from STATE_DIR at module
import (hooks_cli.py:277), so patching STATE_DIR alone left
mine-spawning tests writing PID files under the import-time location
instead of the per-test root. Patch _MINE_PID_DIR too, and create the
state dir so the fixture's 'existing' docstring is accurate. Isolation
still 100 passed/1 skipped; ordering still 165 passed.
…est-isolation

test(hooks): isolate test_hooks_cli from test_cli side effect (MemPalace#1510)
…r-surface

fix(mcp): surface create_tunnel ValueError instead of masking it (MemPalace#1473)
…aceholder

Fix: Write placeholder PID when claiming mine slot
…chunk-size-enforcement

fix(convo_miner): enforce CHUNK_SIZE in paragraph chunker (MemPalace#1534)
milla-jovovich and others added 21 commits May 20, 2026 03:15
…wer tagger

mempalace's init-time entity scanner (`entity_detector.py:276`) already
matches names case-insensitively against corpus content:

    name_line_indices = [i for i, line in enumerate(lines)
                         if name_lower in line.lower()]

That's how a corpus that mentions "aya" all in lowercase still surfaces
"Aya" as a confirmed entity during `mempalace init`.

But the per-drawer tagger in `miner.py:_extract_entities_for_metadata`
was never updated to use the same flag — it walks the same
`known_entities.json` seed list and matches case-sensitively:

    for name in known:
        if re.search(r"(?<!\w)" + re.escape(name) + r"(?!\w)", content):
                                                              # ^ no IGNORECASE
            matched.add(name)

So "Aya" in `known_entities.json` matches "Aya" in drawer content but
silently misses every "aya" / "AYA" mention. Chat transcripts,
voice-typed journals, and any lowercase-style corpus get their drawers
under-tagged in ways that don't show up in init's "we found Aya/Lumi/..."
confirmation list.

## The fix

One regex flag, line 788:

    -if re.search(r"(?<!\w)" + re.escape(name) + r"(?!\w)", content):
    +if re.search(r"(?<!\w)" + re.escape(name) + r"(?!\w)", content,
    +             re.IGNORECASE):

Plus an explanatory comment so the parity with `entity_detector.py:276`
is documented at the call site.

## Empirical proof of the bug (and the fix)

Before this commit, with the actual installed `~/.mempalace/known_entities.json`
containing "Aya", "Lumi":

    Content: "Aya talked to Lumi. Lumi answered."      → tagged 'Aya;Lumi'  ✓
    Content: "aya talked to lumi. lumi answered."      → tagged ''           ✗ MISS
    Content: "AYA mentioned LuMi to ben."              → tagged ''           ✗ MISS

After this commit (same content):

    Content: "Aya talked to Lumi. Lumi answered."      → tagged 'Aya;Lumi'  ✓
    Content: "aya talked to lumi. lumi answered."      → tagged 'Aya;Lumi'  ✓
    Content: "AYA mentioned LuMi to ben."              → tagged 'Aya;Ben;Lumi'  ✓

## Tests

New RED-first test in `tests/test_miner.py`:

    test_entity_metadata_matches_known_names_case_insensitively

Stubs `_load_known_entities` to a controlled `{"Aya", "Lumi"}` seed
and asserts that lowercase + mixed-case mentions both produce the
correctly capitalized tag. Failed on the pre-fix regex (confirmed RED
in CI parity runs across Linux 3.9 / 3.11 / 3.13); passes after the
one-line fix.

## No regressions

Full mempalace test suite: 1920 passed, 1 skipped, 0 failed.
Targeted dependent suites (`test_known_entities_registry`, `test_closets`,
`test_readme_claims`): 123 passed.
ruff check + ruff format clean on pinned 0.15.9.

CI parity replicated locally via Linux containers (Python 3.9, 3.11,
3.13): all three pass the new test and the full `test_miner.py`
sweep (46 tests).

## Why this matters

This is a quiet bug — there's no error message, no failed assertion,
just an empty `entities` field on drawers whose content actually
mentions people. The init phase looks fine ("we detected Aya, Lumi,
Ben") because init does its own case-insensitive scan; mining silently
drops the matches that init's scanner already proved are real.

Personal / journal / chat-style corpora (which use lowercase
conversational style) are the most affected. Code / docs corpora
(which use proper capitalization for names) work either way and saw
no behavior change.
Introduces the missing primitive in mempalace's palace architecture.

## The architectural gap

Mempalace's data model today is Wing → Rooms → Tunnels. Tunnels connect
rooms ACROSS wings (via shared topic words). What's missing — and what
this commit adds — is the connector between ENTITIES (people, projects,
concepts, interests) inside one wing.

The intended full architecture is:

    WING → has DRAWERS (each tagged with entities)
            entities → connected to other entities by HALLWAYS
                       (within-wing, built from drawer co-occurrence)
                       hallways → are the primitive
                                   tunnels → use hallways to spawn
                                             cross-wing connections

A hallway is the structural fact of "these two entities travel together
inside this wing." If Aya and Lumi are mentioned together in 47 drawers
across diary, letters, and ideas rooms, that's a hallway between Aya
and Lumi. If Aya and "consciousness" co-occur in 19 drawers, that's
another hallway. Entities aren't only people — projects, concepts, and
interests share the same entity-tag substrate, so a hallway can connect
a person to a concept (Aya ↔ consciousness) just as easily as person to
person (Aya ↔ Lumi).

`palace_graph.py:253`'s docstring currently calls cross-wing tunnels
"hallways" — that conflation is fixed in a separate naming-disambiguation
follow-up. This commit just introduces the primitive.

## Public API

New module `mempalace/hallways.py`:

  compute_hallways_for_wing(wing, col=None, min_count=2) -> list[dict]
      Query drawers for wing. For each drawer with >=2 entities, every
      unordered pair of entities is one co-occurrence; increment a counter
      and track the room. For each entity pair with count >= min_count,
      materialize a hallway record. Persists to ~/.mempalace/hallways.json.
      Returns records created for this wing (records for other wings
      already on disk are preserved).

  list_hallways(wing=None) -> list[dict]
      Return all hallways, optionally filtered by wing.

  delete_hallway(hallway_id) -> bool
      Remove one record by id. Returns True on success.

  _load_hallways() / _save_hallways(state)
      JSON persistence at ~/.mempalace/hallways.json with 0600 perms on
      POSIX and atomic os.replace writes. Mirrors palace_graph's tunnel
      persistence pattern exactly.

## Data model (hallways.json)

```jsonc
{
  "schema_version": 1,
  "hallways": [
    {
      "id": "hallway_<wing>_<entity_a>_<entity_b>_<sha8>",
      "wing": "wing_aya",
      "entity_a": "Aya",
      "entity_b": "Lumi",
      "co_occurrence_count": 47,
      "rooms": ["diary", "letters", "ideas"],
      "label": "Aya ↔ Lumi (co-occur in 47 drawers across 3 rooms: diary, letters, ideas)",
      "created_at": "2026-05-20T03:30:00Z",
      "created_by": "auto"
    }
  ]
}
```

Hallway IDs are deterministic — `sha256(wing + sorted(entity_a, entity_b))`
truncated. Sorted-pair canonicalization means (Aya, Lumi) and (Lumi, Aya)
produce the same id, so re-runs upsert idempotently and the same drawer
appearing as "Aya;Lumi" or "Lumi;Aya" counts as one co-occurrence (not two).

## Tests

17 RED-first tests in `tests/test_hallways.py`:

Storage:
  - _load_hallways returns [] for missing file
  - _load_hallways returns [] for corrupt JSON
  - _save_hallways + _load_hallways round-trips

Algorithm:
  - returns [] for unknown wing
  - returns [] when no drawer has >=2 entities
  - creates hallway for entity pair when threshold met (Aya↔Lumi in 3 drawers)
  - connects person to concept (Aya↔consciousness — entities aren't only people)
  - respects min_count threshold (rejects 2 co-occurrences when min_count=3)
  - creates deterministic id per entity pair (idempotent re-runs)
  - entity pair is symmetric (Aya;Lumi and Lumi;Aya are the same pair)
  - persists to JSON file
  - tracks rooms across co-occurrences (rooms set vs. count distinction)
  - skips sentinel drawers (file_already_mined bookkeeping noise)

Query API:
  - list_hallways returns all when no filter
  - list_hallways filters by wing
  - delete_hallway removes record and persists
  - delete_hallway unknown id returns False

## Verification

  ruff check mempalace/hallways.py tests/test_hallways.py
    → All checks passed!
  ruff format --check ...
    → 2 files already formatted (pinned 0.15.9)
  pytest tests/test_hallways.py
    → 17 passed
  pytest -q (full mempalace suite)
    → 1936 passed, 1 skipped, 0 regressions caused by this PR

Linux CI parity replicated locally via OrbStack containers
(Python 3.9, 3.11, 3.13): 17/17 hallway tests pass, ruff clean on all
three.

## Not in this commit (deferred)

This PR introduces the primitive — the module, the entity-centric API,
the storage, the tests. It does NOT yet:

  - integrate compute_hallways_for_wing() into miner.py / convo_miner.py /
    format_miner.py post-mine flow (next PR — small one-line addition
    per miner alongside the existing _compute_topic_tunnels call)
  - refactor _compute_topic_tunnels_for_wing to BUILD ON hallways
    (the architecturally meaningful follow-up — own PR)
  - rename palace_graph.py:253's misleading docstring that calls
    cross-wing tunnels "hallways" (cleanup PR)
  - expose hallways through the MCP server (when there's a clear UX
    need for it)

Each follow-up is small enough to review in isolation. Shipping the
primitive first lets reviewers think about the API surface separately
from the integration choices.

Per the design sequence: Wing → Drawer-entities → Hallway → Tunnel.
Hallways come first. This is them.
Wires the hallway primitive (from PR MemPalace#1558) into the project miner so
that every mine that touches a wing also materializes within-wing
entity hallways for that wing. Without this integration, the hallway
module is dead code — no miner triggers it, no hallways ever land in
~/.mempalace/hallways.json.

## What this commit does

1. Adds a module-level import of ``compute_hallways_for_wing`` from
   ``.hallways`` near the top of ``miner.py``. Module-level (not lazy)
   so tests can patch it as ``mempalace.miner.compute_hallways_for_wing``
   — lazy imports inside a function wouldn't expose the seam.

2. In ``_mine_impl``, immediately after the existing
   ``_compute_topic_tunnels_for_wing(wing)`` post-mine block, adds a
   parallel hallway block. The block:

   - calls ``compute_hallways_for_wing(wing, col=collection)``
   - prints the count if any hallways were materialized
   - wraps the whole thing in try/except so a hallway-compute failure
     is logged + degraded, never propagated. Mirrors the tunnel block's
     fault-tolerance pattern exactly. Hallway computation is a derived
     analytic, not load-bearing for the drawer write that already
     committed above.

## Stacking

This PR stacks on PR MemPalace#1558 (which introduces the hallway primitive
module). PR MemPalace#1558 is the prerequisite — without it, the import
``from .hallways import compute_hallways_for_wing`` doesn't resolve.

Base branch for this PR is ``feat/hallways-within-wing-connectors``
(PR MemPalace#1558's branch). When PR MemPalace#1558 merges to develop, GitHub will
auto-update this PR's base to develop and the diff will reduce to
just the miner.py and tests/test_miner.py changes.

## Out of scope (deferred to follow-up PRs)

- ``format_miner.py`` integration: lives on a different branch (PR MemPalace#1555)
  so the parallel call there goes in as a follow-up amendment to that
  branch (or a separate post-merge PR).
- ``convo_miner.py`` integration: convo_miner currently doesn't call
  ``_compute_topic_tunnels_for_wing`` either. Adding both calls is a
  separate concerned PR about convo_miner parity, not just hallways.
- Refactoring ``_compute_topic_tunnels_for_wing`` to BUILD ON hallways
  (rather than computing from raw topic words): the architecturally
  meaningful follow-up that completes the Wing → Drawer-entities →
  Hallway → Tunnel sequence. Real refactor, separate PR.

## Tests

Two new RED-first tests in ``tests/test_miner.py``:

  test_mine_computes_hallways_for_wing_post_mine
      Stubs ``mempalace.miner.compute_hallways_for_wing`` via monkeypatch.
      Runs a real mine into a tmp palace. Asserts the stub was called
      exactly once, with the wing name from mempalace.yaml and a live
      ChromaDB collection (not None).

  test_mine_hallway_failure_does_not_crash_mine
      Stubs the hallway function to raise. Runs a real mine. Asserts
      mine() doesn't propagate, and that the drawer write (which
      happens BEFORE the hallway block) still committed.

Both RED before this commit (AttributeError — module had no attribute
``compute_hallways_for_wing``). Both GREEN after.

## Verification

  ruff check mempalace/miner.py tests/test_miner.py
    → All checks passed!
  ruff format --check ...
    → 2 files already formatted (pinned 0.15.9)
  pytest tests/test_miner.py
    → 47 passed (the 2 new + 45 pre-existing)
  pytest -q (full mempalace suite)
    → 1938 passed, 1 skipped, 0 regressions

Linux CI parity replicated locally via OrbStack containers
(Python 3.9, 3.11, 3.13): 2/2 new integration tests pass, ruff clean
on all three.
…ays-in-miner

feat(miner): integrate compute_hallways_for_wing into post-mine flow
…sitive-entity-matching

fix(miner): mirror init's case-insensitive entity matching in per-drawer tagger
…s-hook-chromadb-deadlock-wi

fix: stale-PID timeout, MCP idle exit, structured errors (MemPalace#1552)
)

- Add MEMPALACE_MINE_TIMEOUT_HOURS (default 2h): PID files now record
  '{pid} {unix_timestamp}'; _mine_already_running() treats alive-but-old
  processes as stale, unblocking queued mines after a ChromaDB hang.
  Backward-compatible: bare-PID files (old format) treated as stale.
- Add MEMPALACE_MCP_IDLE_HOURS (default 8h): daemon watchdog thread in
  mcp_server calls sys.exit(0) after the configured idle period, preventing
  accumulation of stale server processes holding ChromaDB/HNSW file handles.
  Set to 0 to disable.
- Enrich _internal_tool_error() with optional exc parameter: adds
  data: {error_class, message} to JSON-RPC error body so callers can
  distinguish lock contention, ChromaDB transients, and segfaults without
  scraping the message string. MineAlreadyRunning handler in tool_sync()
  adds error_class: 'LockHeldByOtherProcess' to the result dict.
- Update miner._cleanup_mine_pid_file() to parse first whitespace token
  as PID (handles both old and new PID file formats).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Disable mine and MCP idle timeouts when env values are invalid.
- Use bare-PID slot file mtime as the compatibility timestamp instead of treating old slots as infinitely stale.
- Treat malformed PID-slot timestamps as stale without crashing hook execution.
- Use process-level idle watchdog termination so stale MCP servers actually release file handles.
- Restore tenant-isolation assertions accidentally removed from the KG cache test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-configurable

fix(miner): configurable + raised MAX_CHUNKS_PER_FILE (MemPalace#1455)
Adds the architectural counterpart to ``compute_topic_tunnels`` that
materializes cross-wing tunnels from the within-wing hallway records
introduced in PR MemPalace#1558. When an entity (person, project, concept,
interest) has hallways in two wings, an entity tunnel bridges them —
anchored on the entity. This completes the v4 sequence: Wing →
Drawer-entities → Hallway → Tunnel.

Topic tunnels are NOT replaced. Both systems coexist for one release
cycle so existing palaces don't lose tunnels between mines. Deprecation
of topic tunnels is a separate follow-up PR after entity tunnels prove
out in real use.

## What this commit does

1. Adds ``entity_tunnels_for_wing(wing, hallways, label_prefix)`` to
   ``mempalace/palace_graph.py``. Pure function: groups hallway records
   by entity-and-wing, finds entities present in ``wing`` AND ≥1 other
   wing, and emits one ``create_tunnel`` call per (entity, other_wing)
   pair. Uses ``kind="entity"`` and synthetic endpoint room
   ``entity:<name>`` so the new tunnels are distinguishable from
   explicit/topic tunnels at read time but interchangeable with them
   via the standard ``list_tunnels`` / ``follow_tunnels`` API.

2. Adds ``_compute_entity_tunnels_for_wing(wing)`` wrapper to
   ``mempalace/miner.py``. Loads hallway records via
   ``hallways.list_hallways()`` and calls the algorithm. Module-level
   so tests can patch it as ``mempalace.miner._compute_entity_tunnels_for_wing``.

3. Wires the wrapper into ``_mine_impl`` immediately after the existing
   hallway-compute block. Same try/except fault-tolerance pattern as
   the topic-tunnel and hallway blocks — entity-tunnel computation is
   a derived analytic and must never fail a mine.

## Tests (RED-first)

Nine algorithm tests in ``tests/test_palace_graph_tunnels.py``
(new ``TestEntityTunnels`` class):

  test_entity_tunnels_creates_cross_wing_tunnel_for_shared_entity
  test_entity_tunnels_skips_entities_in_only_one_wing
  test_entity_tunnels_counts_entity_in_either_pair_position
  test_entity_tunnels_three_wings_pairwise_from_focus_wing
  test_entity_tunnels_idempotent_on_rerun
  test_entity_tunnels_retrievable_via_list_tunnels
  test_entity_tunnels_empty_hallways_is_noop
  test_entity_tunnels_unknown_wing_is_noop
  test_entity_tunnel_room_does_not_collide_with_literal_room

Two integration tests in ``tests/test_miner.py``:

  test_mine_computes_entity_tunnels_for_wing_post_mine
  test_mine_entity_tunnel_failure_does_not_crash_mine

All 11 RED before this commit (AttributeError on the missing names).
All 11 GREEN after.

## Out of scope (deferred to follow-up PRs)

- ``format_miner.py`` and ``convo_miner.py`` integration: separate PRs
  per the scope discipline used for MemPalace#1560.
- Deprecating ``_compute_topic_tunnels_for_wing``: separate PR after
  entity tunnels prove out in real use.
- Surfacing ``kind="entity"`` in MCP / search-result UI: not yet
  required by any reader; behaviorally interchangeable with the other
  tunnel kinds today.

## Stacking

This PR stacks on PR MemPalace#1558 (which introduces the hallway primitive and
its miner integration). Base branch is
``feat/hallways-within-wing-connectors``. When MemPalace#1558 merges to develop,
GitHub auto-updates this PR's base to ``develop`` and the diff reduces
to just the entity-tunnel additions.

## Verification

  pytest tests/test_palace_graph_tunnels.py::TestEntityTunnels
    → 9 passed (RED before, GREEN after)
  pytest tests/test_miner.py::test_mine_computes_entity_tunnels_for_wing_post_mine
        tests/test_miner.py::test_mine_entity_tunnel_failure_does_not_crash_mine
    → 2 passed (RED before, GREEN after)
  pytest tests/test_palace_graph_tunnels.py
    → 39 passed (no regressions)
  pytest tests/test_miner.py
    → 49 passed (no regressions)
  pytest -q (full mempalace suite)
    → 1949 passed, 1 skipped, 0 regressions
  ruff check mempalace/palace_graph.py mempalace/miner.py tests/
    → All checks passed!
  ruff format --check ...
    → 4 files already formatted (pinned 0.15.9)
…tras + skip symlink tests on Windows

Addresses PR MemPalace#1555 review (Igor) — two bugs that block this PR from
merging cleanly:

1. ``mempalace[extract]`` extras did not pull MarkItDown's per-format
   sub-dependencies. A real PDF after ``pip install mempalace[extract]``
   would raise ``MissingDependencyException`` asking for ``markitdown[pdf]``
   — the code then routed that exception through the generic
   ``except Exception`` and surfaced it as ``SKIP_EXTRACTION_ERROR``,
   stripping the actionable signal from the user.

2. ``test_fringe_broken_symlink`` and ``test_scan_formats_skips_symlinks``
   called ``Path.symlink_to()`` unguarded, which raises ``OSError``
   (``WinError 1314``) on Windows test environments without
   ``SeCreateSymbolicLinkPrivilege`` — surfacing as hard test failures
   before any product code ran.

## What this commit does

1. ``mempalace/format_miner.py`` — adds a new
   ``ExtractionStatus.SKIP_MISSING_FORMAT_DEPS`` enum member and a
   matching catch in ``extract_text`` that fires BEFORE the generic
   ``except Exception`` block. The catch matches by exception type name
   (``type(exc).__name__ == "MissingDependencyException"``) so the static
   import surface doesn't change — MarkItDown stays an optional
   dependency.

2. ``pyproject.toml`` — changes ``[extract]`` to include MarkItDown's
   per-format sub-extras:

     "markitdown[docx,pdf,pptx,xlsx]>=0.1.5; python_version >= '3.10'"

   These pull ``pdfminer-six``, ``pdfplumber``, ``mammoth``, ``lxml``,
   ``python-pptx``, ``openpyxl``, ``pandas`` — the deps each per-format
   converter actually needs at runtime. Verified against MarkItDown
   0.1.5's PyPI metadata (Provides-Extra includes ``pdf``, ``docx``,
   ``pptx``, ``xlsx`` among others). ``.epub`` is handled by base
   markitdown (``EpubConverter`` uses ``beautifulsoup4``, a base
   requirement, so no ``[epub]`` extra is needed — and none exists in
   0.1.5). ``.rtf`` is covered by the existing ``striprtf`` entry.
   ``markitdown[all]`` was NOT used because it pulls audio
   (``pydub``, ``speechrecognition``), YouTube, and Azure deps that
   mempalace does not claim support for and that would bloat the
   install for users.

3. ``tests/test_format_miner.py`` — adds a ``_make_symlink_or_skip``
   helper near the top of the file that wraps ``Path.symlink_to()`` in
   try/except ``OSError`` and calls ``pytest.skip(...)`` on failure.
   Refactors both existing symlink tests
   (``test_fringe_broken_symlink``, ``test_scan_formats_skips_symlinks``)
   to use the helper. Behavior is unchanged on macOS/Linux; on Windows
   without symlink privileges the tests skip cleanly instead of
   spuriously failing.

## Tests (RED-first)

Two new RED-first tests:

  test_extract_text_missing_format_dep_returns_distinct_status
      Patches ``_extract_via_markitdown`` to raise a fake exception
      with ``__name__ == "MissingDependencyException"``. Asserts the
      dispatcher returns ``SKIP_MISSING_FORMAT_DEPS``, not
      ``SKIP_EXTRACTION_ERROR``.

  test_pyproject_extract_extra_pulls_markitdown_format_subdeps
      Parses ``pyproject.toml`` (via ``tomllib`` 3.11+ or ``tomli`` on
      3.9/3.10 — the latter is already a base dependency under the
      ``python_version < '3.11'`` marker). Asserts ``[extract]``
      includes ``pdf``, ``docx``, ``pptx``, ``xlsx`` inside SOME
      ``markitdown[...]`` bracketed group.

Both RED before this commit. Both GREEN after.

Also updates ``test_extraction_status_enum_has_all_documented_codes`` to
include the new ``SKIP_MISSING_FORMAT_DEPS`` code in the documented set,
so the enum-completeness doc-test stays honest.

## Verification

  pytest tests/test_format_miner.py::test_extract_text_missing_format_dep_returns_distinct_status
        tests/test_format_miner.py::test_pyproject_extract_extra_pulls_markitdown_format_subdeps
    → 2 passed (RED before, GREEN after)
  pytest tests/test_format_miner.py::test_fringe_broken_symlink
        tests/test_format_miner.py::test_scan_formats_skips_symlinks
    → 2 passed on macOS (Windows behaviour validated by CI test-windows)
  pytest tests/test_format_miner.py
    → 64 passed, 2 skipped, 0 regressions
  pytest -q (full mempalace suite)
    → 2002 passed, 3 skipped, 0 regressions
  ruff check mempalace/format_miner.py tests/test_format_miner.py
    → All checks passed!
  ruff format --check ...
    → 2 files already formatted (pinned 0.15.9)

## Why no OrbStack run this time

OrbStack runs Linux containers, which have unprivileged symlinks. It
cannot reproduce the Windows ``WinError 1314`` failure mode this
amendment fixes. The only authoritative gate for the symlink fix is
GitHub CI's ``test-windows`` job. For the enum + extras changes, both
are pure-Python / install-time concerns the CI test-linux matrix covers
identically to OrbStack. The ``tomli`` fallback in the new pyproject
test was verified against the base dependency declaration in
``pyproject.toml:32``.
…-wing-connectors

feat(hallways): within-wing entity-to-entity connector primitive
…from-hallways

feat(tunnels): cross-wing entity tunnels derived from hallways
…ers-and-format-coverage

feat(3.3.6): virtual line numbering + format coverage via --mode extract
…#1555

PR MemPalace#1555 (format coverage + virtual line numbering) merged with twelve
inline polish comments from Copilot + gemini-code-assist that weren't
load-bearing enough to block the original ship but are real cleanups.
This PR addresses them.

Twelve items in scope; one item (drawer ID delimiter — Copilot #13) is
deferred to its own dedicated PR because it's a breaking schema change
that requires migration design beyond the scope of a polish PR.

## Behavioral fixes (5 items, RED-tested first)

1. **FileNotFoundError vs broken symlink (Copilot #8).** ``extract_text``
   previously mapped every ``FileNotFoundError`` from ``stat()`` to
   ``SKIP_BROKEN_SYMLINK``. That's misleading for the common case of a
   regular file deleted between scan and extract. Now distinguishes:
   ``SKIP_BROKEN_SYMLINK`` only when ``p.is_symlink()`` is true;
   ``SKIP_UNREADABLE`` otherwise.

2. **``file_already_mined`` extract_mode scoping (Copilot #11, #12).**
   Both call sites in ``mine_formats`` and ``_file_chunks_locked`` now
   pass ``extract_mode="format"``. Previously the format miner could
   falsely treat drawers from project / convo miner on the same source
   file as "already mined" (and vice versa). Scopes idempotency to the
   correct drawer subset.

3. **Sentinel skip for transient missing-dep statuses (Copilot #14).**
   New ``_TRANSIENT_MISSING_DEP_STATUSES`` set + ``_register_skip_sentinel_if_appropriate``
   helper. Skip variants like ``SKIP_NO_MARKITDOWN`` /
   ``SKIP_NO_STRIPRTF`` / ``SKIP_MISSING_FORMAT_DEPS`` /
   ``SKIP_NETWORK_TIMEOUT`` no longer write the "already-mined" sentinel.
   Otherwise installing the missing extra later wouldn't trigger a re-mine.

4. **Outer ``except Exception`` in ``mine_formats`` (Gemini #5).** The
   outer try around the loop previously caught only ``KeyboardInterrupt``,
   leaving any setup-time error (e.g., ``scan_formats`` raising) to
   propagate as a bare traceback. Now catches ``Exception`` defensively,
   logs it, prints a partial-progress summary, and lets the ``finally``
   PID-cleanup run. Mirrors miner.py's belt-and-suspenders pattern.

5. **Thread user's ``chunk_size`` / ``chunk_overlap`` / ``min_chunk_size``
   through to ``chunk_text`` (Gemini #3).** ``MempalaceConfig`` was loaded
   only to validate readability; users who tuned their config saw no
   effect in format-mode mining. Now properly threaded.

## Trivial cleanups (5 items)

6. **Path expanduser in ``extract_text`` (Copilot #7).** ``Path(path)`` →
   ``Path(path).expanduser()`` so CLI inputs like ``~/docs/file.pdf``
   resolve correctly.

7. **Path expanduser+resolve in ``scan_formats`` (Copilot #9).** Same
   fix; ``~/docs`` and relative paths now work consistently.

8. **Use resolved ``format_path`` in ``mine_formats`` (Copilot #10).**
   ``scan_formats(format_dir)`` → ``scan_formats(format_path)`` so the
   already-resolved path is used.

9. **``render_with_line_numbers`` type annotation (Copilot #15).**
   ``text: "str | None"`` reflects the documented + tested ``None``
   handling.

10. **Test + docs claims (Copilot #16, #17, #18).** Stale framings
    removed:
    - ``docs/format-coverage.md`` — 14 fringe cases + "see the file for
      the current test inventory" (no more frozen test count).
    - ``tests/test_line_numbers.py`` — drops "proposed for mempalace
      3.3.6" + "run from the proposal directory" references.
    - ``tests/test_format_miner.py`` — drops "MarkItDown is mocked
      throughout" (live integration tests exist) + proposal-directory
      framing.

## Module-level hoists (enables clean test patching)

- ``MempalaceConfig`` (from ``.config``) hoisted from lazy local import
  to module-level so tests can patch ``mempalace.format_miner.MempalaceConfig``.
- ``chunk_text`` (from ``.miner``) hoisted similarly.

Both follow the pattern PR MemPalace#1565 used for ``compute_hallways_for_wing``.

## Complexity refactor

Extracted ``_print_mine_summary`` from ``mine_formats`` so the orchestrator
stays under the project's ``max-complexity = 25`` ceiling (per
``pyproject.toml [tool.ruff.lint.mccabe]``). Behavior unchanged; pure
extraction.

## Out of scope (intentionally deferred)

- **Drawer ID delimiter collision (Copilot #13)** — ``f"{source_file}{chunk_index}"``
  can theoretically collide (``"/path/a1" + "23"`` == ``"/path/a" + "123"``).
  Fixing this is a breaking schema change to drawer IDs and requires a
  migration plan; will land as its own PR after design.

- The four bot comments that were ALREADY addressed by amendment #3
  before the PR MemPalace#1555 merge (``_SKIP_DIRS`` dedup, ``scan_formats``
  symlink skip, ``source_mtime`` tracking, hall+entities metadata) —
  no action needed; verified during audit.

## Tests (RED-first)

Six new RED-first tests in ``tests/test_format_miner.py``:

  test_extract_text_nonexistent_regular_file_returns_unreadable_not_broken_symlink
  test_mine_formats_passes_extract_mode_format_to_file_already_mined
  test_mine_formats_does_not_write_sentinel_for_skip_no_markitdown
  test_mine_formats_does_not_write_sentinel_for_skip_missing_format_deps
  test_mine_formats_catches_unexpected_exception_and_prints_summary
  test_mine_formats_threads_chunk_size_from_user_config

All six RED before this commit (failures correctly identified the bugs
they're targeting), all six GREEN after.

One existing test (``test_mine_formats_continues_after_per_file_error``)
updated to patch the new module-level binding
``mempalace.format_miner.chunk_text`` instead of the old
``mempalace.miner.chunk_text`` source location, and to accept the
``**kwargs`` the call now passes through. Behavior unchanged.

## Verification

  pytest -q (full mempalace suite)
    → 2065 passed, 3 skipped, 0 regressions
  ruff check mempalace/format_miner.py mempalace/searcher.py tests/
    → All checks passed!
  ruff format --check ...
    → 4 files already formatted (pinned 0.15.9)
  mine_formats complexity
    → ≤ 25 (under the project ceiling)
…g-api-auto-route

feat(convo_miner): auto-route AI tool sessions to wing_api
…ot-feedback

fix(extract): polish PR — address bot review feedback on PR MemPalace#1555
…e#1539)

Four sites passed content to `collection.upsert(documents=[...])` or
`collection.add(documents=[...])` without per-drawer size cap, hitting
the same `RuntimeError: Invalid buffer size` crash class from the
embedding model's attention buffer:

- `general_extractor.extract_memories`: post-classification slicer
  that propagates `memory_type` to every slice. New `chunk_size`
  parameter defaulting to `DEFAULT_CHUNK_SIZE` and resolved from
  `MempalaceConfig.chunk_size` by the caller.
- `diary_ingest.ingest_diaries`: per-entry drawers via existing
  `_split_entries`, with character-chunk fallback for any single
  entry larger than `chunk_size`. New `_diary_drawer_id_entry`
  helper carries (entry_idx, entry_chunk_idx). `chunk_index` in
  metadata is a global counter across the file so
  `searcher._expand_with_neighbors` stitches sibling chunks
  regardless of entry boundary. Upsert is batched atomic per file
  (one call carrying every entry/chunk) so a mid-pass embedding
  failure cannot half-write the day. Auto-purge on full rebuild
  deletes prior-pass drawers via `where={"source_file": ...}`,
  which also migrates pre-MemPalace#1539 legacy `drawer_diary_` IDs as a
  side effect of normal use.
- `mcp_server.tool_diary_write`: split oversized entries into
  bounded per-chunk drawers via a single batched `col.add` (atomic,
  no half-write on embedding failure). `col.add` is intentional:
  `entry_id` is timestamp-based with microsecond precision, so a
  duplicate is a same-microsecond clash that should surface rather
  than silently overwrite.
- `mcp_server.tool_add_drawer`: same crash class on the more common
  add-drawer surface (100 KB sanitize cap, 125x `CHUNK_SIZE`).
  Chunked path mirrors Site 3 with batched atomic `col.upsert`,
  per-chunk `parent_drawer_id` + `chunk_index` metadata, and a
  dual-id idempotency probe (last chunk for atomicity, legacy
  `drawer_id` for pre-MemPalace#1539 single-row backwards-compat). Return
  shape additive: `chunks` always present, `chunk_ids` on the
  chunked path. `tool_get_drawer` / `tool_delete_drawer` against
  the logical handle report "not found" on the chunked path;
  callers iterate `chunk_ids` or query `parent_drawer_id`.

Chunk id width is `:06d` so even a single-digit `chunk_size`
config cannot lex-sort chunks out of order.
…unded-upsert-sites

fix(audit): chunk content before embedding upsert (MemPalace#1539)
…-21)

Brings in upstream's chunked-write paths for tool_add_drawer and
tool_diary_write (MemPalace#1539), file_already_mined extract_mode scoping,
upstream's _is_ai_tool_path/_resolve_wing wing-derivation helpers,
the _claim_mine_slot live-PID-placeholder fix (MemPalace#1443), _start_idle_exit_watchdog
(MemPalace#1552), extract mine mode for office documents, --max-chunks-per-file
(MemPalace#1455), the ruff 0.15.9 pin (matches CI), and 50+ other commits.

Conflict resolutions (7 files):
- .gitignore: kept fork's benchmarks/c_beta/* rules + upstream's .review-logs.
- pyproject.toml: took upstream's ``ruff==0.15.9`` pin, kept fork's
  ``postgres`` optional-deps line.
- mempalace/cli.py: ``--mode`` choices is now ``["projects", "convos",
  "session", "extract"]`` (fork's session + upstream's extract); stacked
  both ``--workers`` (fork) and ``--max-chunks-per-file`` (upstream) on
  ``mempalace mine``.
- mempalace/convo_miner.py: kept fork's ``mine_sessions`` (session
  manifest mining) alongside upstream's new ``_is_ai_tool_path`` /
  ``_resolve_wing`` helpers.
- mempalace/mcp_server.py: merged the room-taxonomy soft warnings
  (fork) with upstream's chunked-write idempotency + base_metadata
  shape for both tool_add_drawer and tool_diary_write; preserved
  session_id metadata; daemon-strict startup path now also calls
  ``_start_idle_exit_watchdog`` in local mode only.
- mempalace/multi_encoder.py: kept the Gemini-mediums sha256 _doc_key
  fix from the in-flight commit.
- tests/test_hooks_cli.py + tests/test_miner.py: union — kept fork's
  daemon-routed test suite + power-resilience tests + upstream's
  _claim_mine_slot live-PID and prefetch_mined_set tests.

Also:
- ruff 0.15.9 reformat applied to 10 files surfaced by the new
  format rules. Matches the CI pin (pyproject + .github/workflows/ci.yml).
- mempalace/miner.py: added ``from typing import Optional`` to support
  the upstream-added ``max_chunks_per_file: Optional[int]`` signatures
  (MemPalace#1455). Lint clean on py3.9-3.13.

Tests: 397 passing in tests/ -k 'age or kg or knowledge_graph or
multi_encoder or writethrough or pending_queue or backend_unreachable
or miner_unit or hooks_cli'. Full suite verification pending CI.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 21, 2026 23:59

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.

@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 significantly expands MemPalace's ingestion capabilities by adding a binary document extraction mode and within-wing entity 'hallways'. It also implements virtual line numbering for drawers, improves HNSW metadata validation, and adds an idle auto-exit watchdog for the MCP server. Key feedback includes addressing a potential ID collision in the format miner, implementing pagination for large collection queries during hallway computation, and scoping deletions to specific extraction modes to prevent unintended data loss.

Comment thread mempalace/format_miner.py
batch_ids: list = []
batch_metas: list = []
for chunk in chunks[batch_start : batch_start + DRAWER_UPSERT_BATCH_SIZE]:
key = (source_file + str(chunk["chunk_index"])).encode()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The ID generation logic is vulnerable to collisions because it concatenates the file path and chunk index without a separator. For example, path/to/file with index 11 and path/to/file1 with index 1 would result in the same hash key (path/to/file11). Using a separator and including the extraction mode (consistent with the fix in convo_miner.py) ensures uniqueness.

Suggested change
key = (source_file + str(chunk["chunk_index"])).encode()
key = f"{source_file}:format:{chunk['chunk_index']}".encode()

Comment thread mempalace/hallways.py

# 1. Query drawers for this wing.
try:
results = col.get(where={"wing": wing}, include=["metadatas"])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

ChromaDB's collection.get() method typically has a default limit (often 10 or 100). Without explicit pagination or a large enough limit, compute_hallways_for_wing will only process a small subset of drawers for the wing, resulting in incomplete hallway data. This should be implemented with a pagination loop using limit and offset, similar to the logic in convo_miner.py or palace.py.

Comment thread mempalace/format_miner.py
return 0, True

try:
collection.delete(where={"source_file": source_file})

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

This unscoped delete will remove all drawers associated with the source_file, including those from other ingest modes (like project mode). This breaks the "per-mode dedup" goal mentioned in the PR description. Since format_miner drawers always carry extract_mode="format", the deletion should be scoped to avoid affecting other modes.

Suggested change
collection.delete(where={"source_file": source_file})
collection.delete(where={"$and": [{"source_file": source_file}, {"extract_mode": "format"}]})

jphein and others added 2 commits May 21, 2026 17:05
Post-merge reconcile — the upstream chunked-write path's single-chunk
return dict lost the warnings field that the fork's room-taxonomy
tests expect. Conflict resolution preserved warnings on the
already-exists and chunked-write paths but missed the single-chunk
path that lives between them.

tests/test_room_taxonomy.py — 19 passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jphein
jphein merged commit abab456 into main May 22, 2026
8 checks passed
@jphein
jphein deleted the merge/upstream-develop-2026-05-21 branch May 22, 2026 00:10
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