Skip to content

fix(tunnels): normalize wing names in topic tunnel lookup for hyphenated dirs#1197

Merged
igorls merged 2 commits intoMemPalace:developfrom
wahajahmed010:fix/1194-hyphenated-wing-tunnels
Apr 27, 2026
Merged

fix(tunnels): normalize wing names in topic tunnel lookup for hyphenated dirs#1197
igorls merged 2 commits intoMemPalace:developfrom
wahajahmed010:fix/1194-hyphenated-wing-tunnels

Conversation

@wahajahmed010
Copy link
Copy Markdown

@wahajahmed010 wahajahmed010 commented Apr 25, 2026

Fix for #1194: Topic tunnels silently skipped for wings with hyphenated dir names

Root Cause

init stores wing names with hyphens/spaces replaced by underscores in mempalace.yaml (e.g. mempalace_public). But tunnel lookup functions (find_tunnels, list_tunnels, follow_tunnels) used the raw directory name (e.g. mempalace-public) as the lookup key, causing silent misses.

Fix

  • Added _normalize_wing() helper that lowercases and replaces hyphens/spaces with underscores, aligning lookup keys with stored metadata
  • Applied normalization in find_tunnels(), create_tunnel(), list_tunnels(), and follow_tunnels()
  • Added logging.warning() calls when tunnel lookups return empty results for filtered queries — no more silent failures
  • Backward compatible: single-token wing names (no hyphens) are unaffected

Testing

  • 4 new tests in TestHyphenatedWingNormalization:
    • test_list_tunnels_filters_hyphenated_wing — verifies list_tunnels matches hyphenated input to stored underscore names
    • test_follow_tunnels_matches_hyphenated_wing — verifies follow_tunnels normalizes correctly
    • test_create_tunnel_normalizes_wing_names — verifies created tunnels use normalized names
    • test_find_tunnels_warns_on_empty_result — verifies warning log on empty results
  • All 13 tests pass

Copy link
Copy Markdown
Collaborator

@bensig bensig left a comment

Choose a reason for hiding this comment

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

Approve. Right shape for the lookup-side defense — small, focused, doesn't conflict with the init-side fix in #1195.

`pytest tests/test_palace_graph_tunnels.py` — 13 passed on this branch.

What landed

  • `_normalize_wing()` helper in `palace_graph.py`
  • `find_tunnels()` normalizes both query keys before lookup
  • `create_tunnel()` normalizes both store keys on creation — important so all new writes go in normalized regardless of caller discipline
  • Logger warning when a filter returns empty — actionable signal vs silent zero

One coordination ask before / after merge

#1195 (mine, on the init side) introduces `normalize_wing_name()` in `mempalace/config.py` as the canonical implementation. The two helpers are byte-identical (`name.lower().replace(" ", "").replace("-", "")`).

If both PRs land, the cleanup is one line: replace this PR's `_normalize_wing` with `from .config import normalize_wing_name`. Same logic, single source of truth. Either:

  • Option A (preferred): rebase this PR after #1195 merges, swap to importing the helper. Minor touch-up.
  • Option B: land both as-is; I'll open a one-commit follow-up that consolidates the two helpers.

Either works — flagging so we don't drift if someone later changes one normalizer and not the other.

Why both PRs should still land

The init-side fix prevents new bad keys from being written. The lookup-side fix here recovers gracefully from the bad keys that already exist in production palaces. Together they're complete defense; either one alone leaves a gap.

Ship it.

@wahajahmed010
Copy link
Copy Markdown
Author

Thanks for the thorough review! Going with Option A — I'll rebase after #1195 merges and swap _normalize_wing for from .config import normalize_wing_name. Makes sense to have a single source of truth for the normalizer. Appreciate the coordination heads-up.

@igorls igorls force-pushed the fix/1194-hyphenated-wing-tunnels branch from 136bf0a to 3474641 Compare April 27, 2026 06:05
@igorls
Copy link
Copy Markdown
Member

igorls commented Apr 27, 2026

Rebased onto current develop (3474641) and pushed.

Conflict resolved in tests/test_palace_graph_tunnels.py — additive: kept this PR's TestHyphenatedWingNormalization class alongside develop's new TestTopicTunnels class from #1184. Both are independent and exercise different surfaces (lookup-side normalization vs. topic-tunnel computation), so they coexist with no logic interaction.

Note for follow-up (after #1195 merges): Igor's review comment on #1195 suggested folding this PR's _normalize_wing helper through from .config import normalize_wing_name once Ben's PR lands — single source of truth for the lower/replace rule across all four wing-slug producers. That swap is mechanical and a one-line change; can either land here as a follow-up commit after #1195 merges, or as a separate cleanup PR. Left as-is for now since normalize_wing_name doesn't exist on develop yet.

Tests: 1422 passed, 1 skipped on the rebased branch. ruff check + format clean. Author preserved on the original commit; committer is mine (force-pushed via maintainer-edit access).

Thanks for the lookup-side fix — together with #1195's producer-side normalization, this closes the hyphenated-wing tunnel loss for both new palaces (Ben's PR) and palaces with bad keys already on disk (this PR).

``def _normalize_wing(wing: str | None) -> str | None`` uses PEP 604
union syntax which requires Python 3.10+ at runtime. The project still
declares ``python_requires=">=3.9"`` and CI runs the test-linux (3.9)
matrix, where every test in ``tests/test_palace_graph*`` errors out
before collection with ``TypeError: unsupported operand type(s) for |``.

Added ``from __future__ import annotations`` so all annotations in
this module are evaluated lazily as strings — the union syntax is then
accepted on 3.9 without needing to rewrite to ``Optional[str]``.

Surfaced after rebasing this PR onto current develop.
@igorls igorls merged commit c3ec708 into MemPalace:develop Apr 27, 2026
6 checks passed
igorls added a commit that referenced this pull request Apr 27, 2026
Bumps every version source from 3.3.3 to 3.3.4:
- pyproject.toml
- mempalace/version.py (canonical)
- .claude-plugin/plugin.json
- .claude-plugin/marketplace.json
- .codex-plugin/plugin.json
- README.md badge

Dates the CHANGELOG section and adds entries for the bug fixes that
landed this cycle (#1135, #1191, #1230, #1231) plus expands the #1194
entry to credit the lookup-side recovery path from #1197.

Pre-tag verification:
- 1441 passed, 1 skipped (full suite minus benchmarks, all platforms)
- ruff check + format clean
- 44/44 in test_version_consistency + test_readme_claims (6-file sync)
- JPH invariant: pyproject.toml + .claude-plugin/plugin.json both
  reference mempalace-mcp
- Wheel build + fresh-venv install: mempalace --version reports 3.3.4,
  mempalace-mcp --help works (catches the v3.3.2-class regression)
@igorls igorls mentioned this pull request Apr 27, 2026
jphein added a commit to jphein/mempalace that referenced this pull request Apr 27, 2026
22 commits from upstream/develop, including:

- HNSW capacity divergence detection + BM25-only sqlite fallback when
  vector layer is unloadable (MemPalace#1222 / MemPalace#1227 — vector_disabled kwarg
  routes search through chromadb-bypass path on segfault risk).
- HNSW index bloat prevention via batch_size + sync_threshold metadata
  on collection create (MemPalace#1191).
- Hooks: always mine the active transcript as convos, additive to
  MEMPAL_DIR (MemPalace#1230 / MemPalace#1231). Restructured into `_get_mine_targets()`
  list approach + separate `_ingest_transcript()`. Daemon-strict gate
  preserved on each entry point.
- Wing-name normalization for hyphenated dirs across miner / convo_miner
  / palace_graph (MemPalace#1194 / MemPalace#1195 / MemPalace#1197).
- `narrow _fix_blob_seq_ids` shim + `repair --mode max-seq-id` for
  legacy 0.6.x BLOB-poisoned palaces (MemPalace#1135).

Conflict resolution:
- README.md: kept fork-shaped narrative, dropped upstream's sweep tip
  injection (per fork-readme handling memory).
- hooks/README.md: adopted upstream's accurate `MEMPAL_DIR` additive
  description; kept `MEMPAL_PYTHON` env-var name (matches actual
  `hooks/*.sh` scripts in both forks).
- mempalace/cli.py: consolidated duplicate `--mode` argparse declarations
  into one with all 4 choices (rebuild/legacy/reorganize/max-seq-id).
- mempalace/hooks_cli.py: adopted upstream's `_get_mine_targets()` +
  `_ingest_transcript()` shape; added `_daemon_strict()` guard at entry
  of `_maybe_auto_ingest`, `_mine_sync`, and `_ingest_transcript` so
  the daemon-strict architecture still skips local writes.
- mempalace/mcp_server.py: kept both `kind=` and `vector_disabled=`
  kwargs on the `search_memories` call.
- mempalace/searcher.py: kept fork's `_count_in_scope`,
  `_sqlite_fallback_and_scope`, `_apply_kind_text_filter` AND upstream's
  `_bm25_only_via_sqlite`. Both `kind` and `vector_disabled` parameters
  on `search_memories`.

Tests: 1510 passing (up from 1366 — upstream brought new test suites).
Scorpion1221 pushed a commit to Scorpion1221/mempalace that referenced this pull request May 2, 2026
Scorpion1221 pushed a commit to Scorpion1221/mempalace that referenced this pull request May 2, 2026
Sync with upstream/develop via cherry-pick of 9 critical bugfixes:
- HNSW index bloat prevention (MemPalace#1191)
- SIGSEGV guards on collection reopen (MemPalace#1262, MemPalace#1289)
- blob-seq marker fast-path (MemPalace#1177)
- palace_graph None-metadata guard (MemPalace#1201)
- palace_graph security tunnels (MemPalace#1168)
- hyphenated wing name normalization (MemPalace#1197)
- searcher _tokenize None guard (MemPalace#1198)
- mcp_server diary topic sanitize (MemPalace#936)

Tests: 1459 default + 113 benchmark + 6/7 stress all pass.
(random-kill stress test was failing on dev pre-merge; not regressed.)
xcarbo added a commit to xcarbo/mempalace that referenced this pull request May 4, 2026
Catches up on a month of upstream work. Highlights pulled in:

- MemPalace#1306 searcher: hybrid candidate union (vector ∪ BM25 reranking pool)
- MemPalace#1325 mcp security: omit absolute paths from tool_get_drawer / tool_status
- MemPalace#1322 chroma: wire quarantine_stale_hnsw to prevent SIGSEGV on stale HNSW
- MemPalace#1320 mcp: forward valid_to / source params in kg_add / kg_invalidate
- MemPalace#1321 cli: honor --palace flag in cmd_init
- MemPalace#1314 kg temporal params fix
- MemPalace#1244 cli: cmd_compress writes to mempalace_closets so palace can read
- MemPalace#1243 mcp: case-insensitive agent name in diary_write/diary_read
- MemPalace#1303 mcp_server: pass embedding_function= on collection reopen
- MemPalace#1076/MemPalace#1077 hooks: quote CLAUDE_PLUGIN_ROOT / CODEX_PLUGIN_ROOT in hooks.json
- Various ruff format passes on touched files

Conflict resolution (CHANGELOG.md only — code files all auto-merged):

- 3.3.5 unreleased section header from upstream kept above 3.3.4
- 3.3.4 section: kept our 2026-04-30 release date; merged upstream's new
  MemPalace#1299 SIGSEGV-on-default-EF entry in alongside our existing topic-tunnels
  (MemPalace#1194/MemPalace#1195/MemPalace#1197), HNSW-bloat (MemPalace#1191), max_seq_id (MemPalace#1135), and
  auto-ingest (MemPalace#1230/MemPalace#1231) entries. Kept our richer topic-tunnels detail
  (upstream's version was a strict subset).

xdev patches preserved (still on this branch, untouched by merge):
- 6ef44cb fix(hooks): route CC transcripts via convo_miner with cwd-based wings
- 3fad61d fix(config): allow leading dash in wing names

Not pushed to origin — run tests locally and decide when to push.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

3 participants