fix: diary wing routing for non-Projects layouts + empty-wing reads (#1145 bugs 1 & 2)#1146
Conversation
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>
There was a problem hiding this comment.
Pull request overview
Fixes two hook/diary routing issues triggered by non-~/Projects/ source layouts and LLM agents defaulting optional parameters to empty strings, improving per-project diary scoping and cross-wing diary reads.
Changes:
- Extend
_wing_from_transcript_path()to derive a project wing for non-~/Projects/transcript folder encodings. - Update
tool_diary_read()to normalizewingusing_sanitize_optional_name()and conditionally apply the wing filter. - Add unit tests covering non-Projects transcript layouts and empty-wing diary reads.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
mempalace/hooks_cli.py |
Adds a second heuristic for deriving project wings from encoded transcript folder names outside ~/Projects/. |
mempalace/mcp_server.py |
Changes diary read filtering to treat empty/whitespace/None wing as “no filter” and builds the where clause conditionally. |
tests/test_hooks_cli.py |
Adds coverage for wing derivation across common non-Projects directory layouts plus a fallback case. |
tests/test_mcp_server.py |
Adds coverage ensuring wing="" reads across wings while explicit wing still scopes correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def tool_diary_read(agent_name: str, last_n: int = 10, wing: str = ""): | ||
| """ | ||
| Read an agent's recent diary entries. Returns the last N entries | ||
| in chronological order — the agent's personal journal. | ||
|
|
||
| When ``wing`` is provided, reads from that wing instead of the | ||
| agent's default ``wing_<agent_name>`` wing. This lets hooks | ||
| direct diary reads to a project-specific wing derived from | ||
| the transcript path. | ||
| ``wing`` behavior matches ``mempalace_search`` (#1097): | ||
| empty-string / whitespace-only / ``None`` means "no wing filter" — | ||
| return entries from any wing this agent has written to. An explicit | ||
| non-empty wing scopes reads to that wing only, e.g. for hooks that | ||
| direct diary reads to a project-specific wing derived from the | ||
| transcript path. Resolves #1145 bug 2. | ||
| """ | ||
| try: | ||
| agent_name = sanitize_name(agent_name, "agent_name") | ||
| if wing: | ||
| wing = sanitize_name(wing) | ||
| wing = _sanitize_optional_name(wing, "wing") | ||
| except ValueError as e: |
There was a problem hiding this comment.
tool_diary_read now treats the default wing="" (i.e., callers omitting wing) as “no wing filter”, but the tool schema still documents “If omitted, reads from wing_{agent_name}”, and this is a behavioral change from the previous default-to-agent-wing behavior. If you want to support both behaviors (omit => agent default, explicit empty string/whitespace => no filter), consider using a sentinel default (e.g., wing=_UNSET) to distinguish “not provided” from "", and update the TOOLS["mempalace_diary_read"] schema description to match the actual semantics.
| try: | ||
| # Build filter conditions: agent + diary room always apply; wing is | ||
| # optional (matches #1097's empty-string semantics for tool_search). | ||
| conditions = [ | ||
| {"room": "diary"}, | ||
| {"agent": agent_name}, | ||
| ] | ||
| if wing: | ||
| conditions.append({"wing": wing}) | ||
| where = conditions[0] if len(conditions) == 1 else {"$and": conditions} | ||
| results = col.get( | ||
| where={ | ||
| "$and": [ | ||
| {"wing": wing}, | ||
| {"room": "diary"}, | ||
| {"agent": agent_name}, | ||
| ] | ||
| }, | ||
| where=where, | ||
| include=["documents", "metadatas"], | ||
| limit=10000, | ||
| ) |
There was a problem hiding this comment.
With wing optional (and potentially unset/empty), tool_diary_read can now fetch across all wings for an agent, but it still does col.get(..., limit=10000) and then sorts/slices client-side. This risks silently truncating results (there’s an existing note elsewhere about a 10K get() limit) and makes the total field inaccurate when there are >10K diary entries. Consider paginating get() (offset loop) or fetching only metadatas first to identify the latest last_n entry IDs, then retrieving documents for just those IDs; this avoids large reads and removes the 10K correctness hazard.
|
Closing as duplicate — we filed #1146 and @igorls filed #1147 within four minutes of each other covering the same two bugs. Their PR is the right vehicle for 3.3.3:
Thanks @igorls for the smoke test + quick turnaround on both the findings and the fix. Re the Bug 3 retraction — confirmed on my side: No action needed on my end. |
…alace#1147 Filed MemPalace#1146 at 02:36 UTC, @igorls filed MemPalace#1147 at 02:40 UTC covering the same two bugs from MemPalace#1145. Their approach is cleaner (primary regex pattern-matches Claude Code's `.claude/projects/-` location directly; ours used `-Projects-<x>` first with fallback). Bug 3 retracted — confirmed env inheritance works via `subprocess.Popen` default, no propagation gap. Fork main keeps `34e36ae` for local use until upstream merges MemPalace#1147 as part of v3.3.3; next `develop→main` merge will replace our version with upstream's. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…g + empty-wing reads) 4 commits pulled (8ac98f0, 6fcfd34, 1fd16da, d158375) landing @igorls's MemPalace#1147 which fixes the same two MemPalace#1145 bugs we addressed in our 34e36ae (closed as duplicate by MemPalace#1146 close). Conflict resolution — took upstream on all 4 overlapping files: - mempalace/hooks_cli.py — upstream's `.claude/projects/-` primary regex with `-Projects-<x>` legacy fallback (functionally equivalent to ours, slightly cleaner ordering) - mempalace/mcp_server.py — upstream's tool_diary_read empty-wing normalization (essentially identical to ours; docstring is clearer) - tests/test_hooks_cli.py — upstream's tests (3 new cases: non-Projects layout, macOS Users/, nested deep). Lost our bare-folder-no-delimiters test, but upstream's coverage is adequate. - tests/test_mcp_server.py — upstream's diary_read tests (cover wing="" spans all wings + explicit wing filter). Overlap with ours. Post-merge: 1094 passed + 10 expected fork-ahead MemPalace#19 failures in test_claude_plugin_hook_wrappers.py (venv-aware hooks vs PATH-only test contract — documented, unchanged). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses two of the three findings @igorls flagged on #1145 from the v3.3.3 smoke test. Batched into one PR because both are in the same file class and share the "LLM agent / non-
~/Projects/user defaults to empty string" failure mode. Bug 3 (Stop-hook auto-mineMEMPALACE_PALACE_PATHenv propagation) lives in a different code path and is deferred to a follow-up PR pending local repro.Bug 1 —
_wing_from_transcript_pathis~/Projects/-centricThe heuristic only matched
-Projects-<project>in the encoded transcript folder name. Users whose source lives outside~/Projects/—~/dev/,~/src/,~/code/, or any other Linux convention — got transcript paths like~/.claude/projects/-home-<user>-dev-<Parent>-<project>/*.jsonlwith no-Projects-segment, fell through to thewing_sessionsfallback, and lost the per-project diary scoping #659 was meant to deliver.Fix: keep the explicit
-Projects-<x>match first (preserves existing behavior / test coverage), 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 — Claude Code's encoding puts it there for any path, not just under~/Projects/.Bug 2 —
tool_diary_readdidn't adopt #1097's empty-string pattern#1097 fixed
mempalace_searchto treatwing=""/room=""as "no filter" (LLM agents frequently default optional string parameters to"").tool_diary_readstill defaulted an emptywingtowing_<agent_name>, silently hiding entries written to named wings — e.g., the per-project wings from #659 that the stop-hook now derives.Fix: route
wingthrough the_sanitize_optional_name()helper#1097introduced so empty / whitespace /Noneall coerce toNoneand drop out of the query. An explicit non-empty wing still scopes to that wing. Build thewhereclause from a list of required conditions (room=diary,agent=<name>) and conditionally append the wing filter — matches the patterntool_list_drawersalready uses.Tests
Six new tests, all green. Full suite: 1067 passed (this branch, from
upstream/develop).tests/test_hooks_cli.py— four_wing_from_transcript_pathcases:test_wing_from_transcript_path_non_projects_layout— igor's specific scenario (~/dev/<Parent>/<project>)test_wing_from_transcript_path_src_layout—~/src/<app>test_wing_from_transcript_path_code_layout—~/code/<app>test_wing_from_transcript_path_bare_folder_no_delimiters_falls_back— genuinely unstructured parent →wing_sessionstests/test_mcp_server.py::TestDiaryTools— twotool_diary_readcases:test_diary_read_empty_wing_returns_across_wings—wing=""returns entries from any wing (the bug as reported)test_diary_read_explicit_wing_still_filters—wing="wing_a"still scopes correctly; regression guard against overcorrecting the empty-string fixNot in this PR
Bug 3 from #1145 (Stop-hook auto-mine subprocess ignores
MEMPALACE_PALACE_PATHenv) — env-propagation bug in a different subprocess-launch site, same file but different narrative/tests. Deferred to a follow-up PR once I've reproduced locally and confirmed root cause. Happy to bundle if reviewers prefer one PR; say the word.🤖 Generated with Claude Code