Skip to content

fix: treat empty string as no filter in mempalace_search wing/room#1097

Merged
bensig merged 2 commits intoMemPalace:developfrom
KunalG67:fix/search-empty-string-filter
Apr 23, 2026
Merged

fix: treat empty string as no filter in mempalace_search wing/room#1097
bensig merged 2 commits intoMemPalace:developfrom
KunalG67:fix/search-empty-string-filter

Conversation

@KunalG67
Copy link
Copy Markdown
Contributor

Fixes #1084

What does this PR do?

Treats empty string as None (no filter) in _sanitize_optional_name.
Previously, mempalace_search rejected empty-string wing/room values
with "must be a non-empty string" even though the schema marks them optional.
This broke LLM agents that default to filling every declared parameter with "".

How to test

Call mempalace_search with wing: "" and room: "" - should return
results instead of an error. Quick unit check:

from mempalace.mcp_server import _sanitize_optional_name
assert _sanitize_optional_name("") is None
assert _sanitize_optional_name(None) is None
assert _sanitize_optional_name("main") == "main"

Checklist

  • Tests pass (python -m pytest tests/ -v)
  • No hardcoded paths
  • Linter passes (ruff check .)

@sha2fiddy
Copy link
Copy Markdown
Contributor

Hey @KunalG67, your one-liner handles the exact reproduction case from #1084.

Heads-up on one edge case: whitespace-only strings (" ", "\t", "\n") still raise ValueError because sanitize_name at mempalace/config.py:27 rejects any string where not value.strip(). Agents trip on this the same way they produce "" (trailing space, stray tab). I've seen it in practice.

Changing the guard to value is None or not value.strip() covers both.

I've got four regression tests for this sitting on a local branch (empty + whitespace on list_rooms, list_drawers, search). Want me to paste the diff so you can fold them in, or send as a follow-up after yours lands?

@KunalG67
Copy link
Copy Markdown
Contributor Author

@sha2fiddy Thanks! Updated to not value.strip() to cover whitespace only strings too. Feel free to paste the test diff ,happy to fold them in

@sha2fiddy
Copy link
Copy Markdown
Contributor

sha2fiddy commented Apr 22, 2026

Thanks for jumping on the whitespace case.

The 4 tests below drop into tests/test_mcp_server.py right before test_wal_redacts_sensitive_fields. They reuse the _patch_mcp_server helper already in that file. I ran them locally against your e27967b head, all 4 pass.

    # ── #1084: empty/whitespace filters normalize to None ───────────────

    def test_search_accepts_empty_wing_as_no_filter(self, monkeypatch, config, kg):
        """Empty-string wing must be treated as 'no filter', not a validation error.

        LLM agents that eagerly fill every declared parameter pass ``""`` to
        mean "no filter". Regression guard for #1084.
        """
        _patch_mcp_server(monkeypatch, config, kg)
        from mempalace import mcp_server

        captured = {}

        def _fake_search(query, palace_path, wing, room, n_results, max_distance):
            captured["wing"] = wing
            captured["room"] = room
            return {"results": []}

        monkeypatch.setattr(mcp_server, "search_memories", _fake_search)

        result = mcp_server.tool_search(query="JWT", wing="")
        assert "error" not in result
        assert captured["wing"] is None

    def test_search_accepts_whitespace_wing_as_no_filter(self, monkeypatch, config, kg):
        """Whitespace-only wing collapses to None — same rationale as empty string."""
        _patch_mcp_server(monkeypatch, config, kg)
        from mempalace import mcp_server

        captured = {}

        def _fake_search(query, palace_path, wing, room, n_results, max_distance):
            captured["wing"] = wing
            captured["room"] = room
            return {"results": []}

        monkeypatch.setattr(mcp_server, "search_memories", _fake_search)

        result = mcp_server.tool_search(query="JWT", wing="   ", room="\t")
        assert "error" not in result
        assert captured["wing"] is None
        assert captured["room"] is None

    def test_list_drawers_accepts_empty_room_as_no_filter(
        self, monkeypatch, config, palace_path, seeded_collection, kg
    ):
        """Empty-string room on list_drawers must not raise — regression for #1084."""
        _patch_mcp_server(monkeypatch, config, kg)
        from mempalace.mcp_server import tool_list_drawers

        result = tool_list_drawers(wing="", room="")
        assert "error" not in result
        assert "drawers" in result

    def test_list_rooms_accepts_empty_wing_as_no_filter(
        self, monkeypatch, config, palace_path, seeded_collection, kg
    ):
        """Empty-string wing on list_rooms must not raise — regression for #1084."""
        _patch_mcp_server(monkeypatch, config, kg)
        from mempalace.mcp_server import tool_list_rooms

        result = tool_list_rooms(wing="")
        assert "error" not in result
        assert result["wing"] == "all"

Rename or trim whatever doesn't fit your style.

@bensig bensig merged commit 9947ad0 into MemPalace:develop Apr 23, 2026
andrew-gamakon pushed a commit to Gamakon/mempalace that referenced this pull request Apr 24, 2026
…emPalace#1097)

* fix: treat empty string as no filter in mempalace_search wing/room

* fix: also treat whitespace-only strings as no filter
jphein pushed a commit to jphein/mempalace that referenced this pull request Apr 24, 2026
Restore-integrity release. Unbreaks fresh `pip install mempalace` from
v3.3.2 by re-tagging current develop, which carries both the plugin.json
consumer (shipped in 3.3.2) and the matching mempalace-mcp entry point
in pyproject.toml (added on develop ~10h after the 3.3.2 tag via MemPalace#340
by @messelink). MemPalace#1093 diagnosed by @jphein.

Bumps (all 5 sources agree per Version Guard / CLAUDE.md):
- mempalace/version.py              3.3.2 → 3.3.3
- pyproject.toml                     3.3.2 → 3.3.3
- .claude-plugin/plugin.json         3.3.2 → 3.3.3
- .claude-plugin/marketplace.json    3.3.2 → 3.3.3
- .codex-plugin/plugin.json          3.3.2 → 3.3.3
- CHANGELOG.md                        new [3.3.3] entry

No code changes. The fix for MemPalace#1093 is already on develop via merged PRs
MemPalace#340, MemPalace#1021, MemPalace#851, MemPalace#942, MemPalace#833, MemPalace#673, MemPalace#661, MemPalace#659, MemPalace#1097, MemPalace#1051, MemPalace#1001,
MemPalace#945.

Branch name intentionally outside the `release/*` ruleset so follow-up
CI-fix commits aren't gated behind a nested PR. (Supersedes MemPalace#1143 —
closed for exactly that reason after it missed 3 of 5 version files.)

Smoke-tested locally from a fresh develop clone:
  grep mempalace-mcp pyproject.toml .claude-plugin/plugin.json   # both ✓
  python -m build --wheel                                        # ✓
  pip install …-py3-none-any.whl                                 # ✓
  which mempalace-mcp                                            # ✓
  mempalace-mcp --help                                           # ✓
jphein added a commit to jphein/mempalace that referenced this pull request Apr 24, 2026
…safe graph cache

31 commits including:
- v3.3.3 release (restores pip install integrity post-v3.3.2 MemPalace#1093 regression)
- MemPalace#1097 empty-string filter normalization in mempalace_search
- MemPalace#659 diary wing parameter (our fork PR, now upstream)
- MemPalace#851/MemPalace#1097/MemPalace#1021/MemPalace#661/MemPalace#673 incorporated
- website Crystal Lattice brand refresh
- thread-safe graph cache in palace_graph.py

Conflict resolutions (10 files):
- README.md            keep fork version; bump badge 3.3.1 → 3.3.3 for test compat
- hooks/README.md      keep fork silent/block architecture docs; keep MEMPAL_PYTHON
                       (correct for legacy hook, upstream's rename is stale)
- examples/HOOKS_TUTORIAL.md  same treatment
- mcp_server.py        take upstream's sanitize_name(wing) — strictly better than
                       our crude lowercase+underscore normalization
- miner.py             keep fork 10K batch size + comma formatting on status;
                       adopt upstream's pagination rationale comment
- palace_graph.py      take upstream entirely — thread-safety improvements layered
                       on top of our MemPalace#661 (which upstream already merged)
- hooks_cli.py         take upstream (Windows path-separator compat in
                       _wing_from_transcript_path, Codex CLI format in
                       _extract_recent_messages), then re-apply fork-ahead: use
                       _wing_from_transcript_path in _ingest_transcript instead
                       of hardcoded "sessions" — keeps transcript mining coherent
                       with the diary wing derivation from MemPalace#659
- tests/test_hooks_cli.py  take upstream's updated wing-kwarg assertions and
                       new test_stop_hook_derives_wing_from_transcript_path;
                       take upstream's mock-based security test (simpler than
                       our three-way assertion, same property tested)

Post-merge test state:
- 1096 passed, 10 failed in tests/test_claude_plugin_hook_wrappers.py
- The 10 failures are the fork-ahead MemPalace#19 divergence already documented in
  CLAUDE.md: our venv-aware hooks use `dirname`/`cat` which the test's
  scrubbed-PATH environment doesn't provide. Same class that correctly caught
  MemPalace#1115 and led us to withdraw it pending MemPalace#1069 arbitration. Expected.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jphein added a commit to jphein/mempalace that referenced this pull request Apr 24, 2026
Addresses two of the three findings @igorls flagged in MemPalace#1145 from the
v3.3.3 smoke test. Same file class, same "LLM agent / edge-case user
defaults to empty string" failure mode — batched into one PR.

## Bug 1: `_wing_from_transcript_path` regex was too narrow

The regex only matched `-Projects-<project>` in the encoded transcript
folder name. Users whose code lives outside `~/Projects/` — `~/dev/`,
`~/src/`, `~/code/`, or any non-Projects convention — got transcript
paths like `~/.claude/projects/-home-<user>-dev-<Parent>-<project>/...`
with no `-Projects-` segment, fell through to `wing_sessions`, and lost
the per-project diary scoping MemPalace#659 was meant to deliver.

Fix: add a second strategy. Keep the explicit `-Projects-<x>` match
first (preserves existing test coverage and behavior for code under
`~/Projects/`), then fall back to the last `-`-delimited segment of
the encoded folder name. That segment is the actual project directory
name regardless of source layout.

Four new tests cover the non-Projects scenarios igor raised plus a
genuine fallback case (parent folder without `-` delimiters → still
`wing_sessions`). Existing tests for the `-Projects-<x>` path stay
green.

## Bug 2: `tool_diary_read` ignored MemPalace#1097's empty-string pattern

`MemPalace#1097` fixed `mempalace_search` to treat `wing=""` / `room=""` as
"no filter" — LLM agents frequently default optional string parameters
to `""`. `tool_diary_read` still defaulted an empty `wing` to
`wing_<agent_name>`, which silently hid entries written to named
wings (e.g. the per-project wings from MemPalace#659).

Fix: route `wing` through `_sanitize_optional_name()` (the helper
MemPalace#1097 introduced) so empty / whitespace / `None` all coerce to `None`
and drop out of the query filter. An explicit non-empty wing still
scopes to that wing — verified by a regression test.

Build the `where` clause from a list of required conditions
(`room=diary`, `agent=<name>`) and conditionally append the wing
filter — matches the pattern `tool_list_drawers` already uses.

## Bug 3 (Stop-hook auto-mine `MEMPALACE_PALACE_PATH` env
   propagation) — not in this PR

Deferred to a follow-up PR pending local reproduction of the env-
propagation behavior. Will file separately once I've confirmed root
cause per the plan in my MemPalace#1145 comment.

## Tests

1067 passed on this branch (branched from `upstream/develop`). Six
new tests:
  - tests/test_hooks_cli.py — four new `_wing_from_transcript_path`
    cases (`~/dev/`, `~/src/`, `~/code/`, bare fallback)
  - tests/test_mcp_server.py — two new `tool_diary_read` cases
    (empty-wing reads across wings, explicit wing still filters)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gut-puncture pushed a commit to gut-puncture/mempalace that referenced this pull request Apr 24, 2026
MemPalace#1097 fixed mempalace_search to treat empty-string wing/room as
no filter, matching how LLM agents default to filling every optional
parameter with ''. The same pattern wasn't applied to diary_read:
passing wing='' defaulted to wing_<agent_name>, siloing away entries
that hooks had written to project-derived wings per MemPalace#659.

When wing is empty/omitted, filter only on agent + room=diary so
callers get a unified view of the agent's journal across every wing
it has written to. Explicit wing=<name> continues to scope reads
to that wing only.

Adds test covering empty-wing read after writing to both the default
and a non-default wing.
jphein added a commit to jphein/mempalace that referenced this pull request Apr 24, 2026
… state

Three stale sections updated:

- Fork change queue: row 8 (.blob_seq_ids_migrated marker) struck
  through → FILED as MemPalace#1177. Two new rows added for segfault fixes
  discovered today (MemPalace#1171 concurrent-write lock, MemPalace#1173 quarantine in
  make_client) that weren't in the queue because the bugs surfaced
  today, not during the original 2026-04-21 triage.

- Open upstream PRs: was showing 3 of 10 PRs. Now shows all 10 with
  current CI/review state. All rebased onto current upstream/develop
  and MERGEABLE as of today.

- Merged since v3.3.1: added v3.3.3 release (2026-04-24) with its
  constituent merges — MemPalace#942, MemPalace#833, MemPalace#1097, MemPalace#1145, MemPalace#1147, MemPalace#1148/1150/1157
  entity-detection overhaul (via @igorls's MemPalace#1175 stacked-PR rescue),
  MemPalace#1166 palace-path security, MemPalace#340/MemPalace#1093 install regression, plus MemPalace#851
  from the 2026-04-22 batch.
gnusam pushed a commit to gnusam/mempalace-pgsql that referenced this pull request Apr 25, 2026
Port of upstream 9947ad0 (PR MemPalace#1097) — LLM agents tend to fill optional
parameters with "" rather than omitting them. Without this normalization,
wing="   " (whitespace-only) is truthy in searcher.py and gets passed
as a literal filter, scoping the search to a non-existent empty wing
and returning zero results.

Adapted: upstream centralizes through a _sanitize_optional_name helper
that we don't have. Inlined the strip-and-empty-as-None at the entry
point of tool_search instead, which is the only call site with this
pattern in our fork (tool_list_rooms uses `if wing:` already-falsy on
"", and tool_find_tunnels accepts no LLM-driven optional filters).

Upstream: MemPalace@9947ad0

Co-authored-by: Kunal Garhewal <kunalgarhewal13@gmail.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gnusam pushed a commit to gnusam/mempalace-pgsql that referenced this pull request Apr 25, 2026
…stop hook

Combined port of upstream df3ee28 (PR MemPalace#659) and 1fd16da (PR MemPalace#1145).
The two upstream commits chain on the same diary scoping feature, so
landing them as one fork commit keeps the reasoning in one place.

mcp_server.py
  * tool_diary_write(): new optional `wing` param. Empty/whitespace
    falls back to the existing wing_<agent_name> default.
  * tool_diary_read(): new optional `wing` param.
      - When wing is provided → filter by wing + room=diary + agent.
      - When wing is empty/whitespace → span all wings this agent has
        written to (room=diary + agent only). Matches the LLM-friendly
        empty-string pattern from PR MemPalace#1097.
    The agent filter is applied unconditionally so callers sharing a
    project wing can only see their own entries (security note from
    bensig's PR MemPalace#659 review).
  * TOOLS schema: `wing` exposed as optional on both diary tools with
    descriptions explaining the per-project use case.

hooks/mempal_save_hook.sh
  * Derive PROJECT_WING from $TRANSCRIPT_PATH. Claude Code transcripts
    live under ~/.claude/projects/-encoded-project-folder/<session>.jsonl;
    the final dash-separated token is a stable per-project handle.
    Adapted from upstream d158375 (Linux/cross-platform variant of the
    macOS-only path heuristic).
  * Append `wing="<derived>"` hint to the AUTO-SAVE block reason so the
    AI files diary entries via mempalace_diary_write under the project
    wing instead of the agent's bucket.
  * Reason is now emitted via python3 -c (json.dumps) so quoting the
    derived wing into the JSON string is safe.

Adapted: upstream's df3ee28 also touches mempalace/hooks_cli.py with a
_wing_from_transcript_path() helper. This fork has no hooks_cli.py
(bash-only hooks), so the derivation lives directly in the bash hook.
sanitize_name() is also not in this fork; minimal validation is done
inline (strip + truthiness check) which matches the existing
tool_search wing-handling pattern.

7 tests added covering: wing override on write, default fallback,
whitespace fallback, wing+agent filter on read, empty-wing/whitespace
spanning all wings.

Upstream:
  MemPalace@df3ee28
  MemPalace@1fd16da
  MemPalace@d158375 (hook-derivation
    portion adapted to bash)

Co-authored-by: Jeffrey Hein <jp@jphein.com>
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>
gnusam pushed a commit to gnusam/mempalace-pgsql that referenced this pull request Apr 25, 2026
…r helper

Follow-up to eb27e00 (port of upstream PR MemPalace#1097). The original port only
fixed tool_search; tool_list_rooms and tool_find_tunnels still treated
whitespace-only filters as truthy, which let an LLM-supplied wing="   "
turn into a literal `WHERE wing = '   '` SQL filter that returned zero
rows.

Extracts the strip-and-empty-as-None logic into _normalize_optional_filter
and routes every LLM-callable tool with optional filter args through it:

  tool_search(wing, room)          — replaced inline normalization
  tool_list_rooms(wing)            — was the latent bug
  tool_find_tunnels(wing_a, wing_b) — was the latent bug
  tool_diary_write(wing)           — replaced inline normalization
  tool_diary_read(wing)            — replaced inline normalization

The helper also strips leading/trailing whitespace on real values, so
wing="  wing_code  " is now treated as wing="wing_code" instead of
silently matching nothing.

Six tests added: helper itself (3 cases), tool_list_rooms with empty +
real wing (2 cases), tool_find_tunnels with empty + real wings (1 case).

Found during second-order analysis of the 2026-04-25 upstream sync —
caught before any LLM agent hit the latent bug in the wild.
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.

bug: mempalace_search rejects empty-string wing/room with "must be a non-empty string" even though schema marks them optional

3 participants