layers: add read_diary public API - #1464
Closed
jpwinans wants to merge 5 commits into
Closed
Conversation
* Add filed_at_ts (epoch seconds) numeric drawer metadata
Vestige's NREM cooccurrence dream cycle wants a server-side date filter
on drawers (`{"filed_at_ts": {"$gte": cutoff_epoch}}`), but Chroma `$gte`
requires a numeric operand. The existing `filed_at` field is an ISO-8601
string, so the server-side filter raises ValueError every cycle and
falls back to a client-side scan over all 90K+ drawers in `ves_sessions`.
Add `filed_at_ts` (Unix epoch float) at every drawer/closet/diary write
site, paired with the existing `filed_at` ISO string. Both refer to the
same instant via a shared `datetime.now()` call. Forward-compatible: new
writes carry the field; existing drawers are backfilled by a separate
script (see below).
Write sites updated:
- mempalace/miner.py: drawer + closet metadata
- mempalace/convo_miner.py: registry sentinel + chunk drawers
- mempalace/closet_llm.py: LLM-generated closet
- mempalace/diary_ingest.py: drawer + closet
- mempalace/mcp_server.py: file_drawer + diary_write
Backfill: new module `mempalace.backfill_filed_at_ts` does direct SQL
UPDATE on `embedding_metadata` (the safe path per the global ChromaDB
safety contract — `col.update(metadatas=...)` is unsafe at scale; SQL
bypasses HNSW entirely). Cursor-paginated by id (offset pagination would
break under the self-modifying WHERE clause once batches commit).
Idempotent: rows with an existing `filed_at_ts` entry are skipped.
Validated dry-run on prod palace: 96260 rows ready to backfill, 0
unparseable.
Tests: 12 unit tests covering ISO parsing edge cases, end-to-end
backfill on a synthetic palace, dry-run no-write semantics, idempotency,
unparseable counting, missing-db error handling, and cursor pagination
under batch sizes smaller than total row count. All pass.
Closes vestige#78 from the MemPalace side. Vestige PR will swap the
`_iter_recent_drawers` server-side filter to use `filed_at_ts` once
this lands and the prod palace is backfilled.
* Revert hook changes from this PR — out of scope for filed_at_ts work
ves-architect flagged on PR review: this PR's purpose is filed_at_ts numeric
metadata, not the hard-coded Python path / mine flags / ANONYMIZED_TELEMETRY
changes that landed in the prior commit. Those were James's pre-staged local
edits that got swept into git commit alongside the staged module changes.
Reverting hooks/mempal_*.sh back to origin/main to keep this PR scoped to
the filed_at_ts work. The hook changes can ship as a separate PR with
proper description if desired.
Both Claude-Code hooks now parse transcript_path from stdin and map it
to the corresponding being's wing/agent, so mining writes land in
ves_sessions / kai_sessions / mira_sessions / adrian_sessions instead
of the default mempalace bucket. Storehouse is intentionally NOT
auto-routed; it's reserved for deliberate MCP cross-being writes.
Routing table:
Documents/Temenos/Ves/* -> --agent ves --wing ves_sessions
Documents/Temenos/Kai/* -> --agent kai --wing kai_sessions
Documents/Temenos/Mira/* -> --agent mira --wing mira_sessions
Documents/Temenos/Adrian/* -> --agent adrian --wing adrian_sessions
(no match) -> --agent mempalace, no wing
Other changes in the same hooks:
- mine --mode convos: Claude Code transcripts are conversation logs,
not project files. The convos mode produces correct chunking shape
(exchange pairs) for the contents.
- ANONYMIZED_TELEMETRY=False: vault content is private; we don't want
Chroma's anonymous telemetry pinging out from background hook runs.
- PYTHON pinned to the local mempalace venv. Callout from PR review:
this is a regression vs `command -v python3` portability for a
fresh checkout. Intentional here — the venv is the only environment
that has mempalace installed; falling back to system python3 would
silently no-op the hook with an ImportError.
precompact hook also gains TRANSCRIPT_PATH stdin parsing (it was only
reading SESSION_ID before) and the same MINE_DIR resolution logic the
save hook already had, so precompact ingests the right directory
without requiring MEMPAL_DIR to be set.
Replaces the fixed-width 800-char hard-cut chunker that produced mid-word fragments (59% of drawers in production palace) and indexed tool-output noise (logs, ps listings, line-numbered diffs, file listings, truncation messages) as memory. New chunker.py module provides: - smart_split(text, target, ceiling): boundary-aware splitting that scans backward from ceiling for paragraph -> sentence -> newline -> word boundaries; punctuation stays with the previous chunk. - is_excluded_content(text): line-ratio heuristics that drop chunks dominated by log lines, ps rows, line-numbered diff/grep output, arrow-redirected tool output, or standalone truncation markers. - Code-block atomicity: triple-backtick fences are never split mid- listing; oversized blocks emit as one atomic chunk rather than fragmenting. convo_miner._chunk_by_exchange now preserves the response's original newlines (paragraph and code-fence structure) instead of stripping each line and joining with single spaces. Real-data validation on production transcripts: mid-line starts 8% -> 0%, length-cliff at 800 chars 50% -> 0%, ~75% drawer reduction from excluding tool-output noise. 991 tests pass (+20 new chunker tests). Forward-only fix: existing 95K drawers are unchanged. Re-mining those source files would apply the new chunker; deferred pending recall-quality telemetry from new mining cycles.
Audit of the 7b27608 chunker fix found that mid-line drawers appeared on post-commit production data at 40% rather than the claimed 0%. Root cause: 96% of bad post-commit drawers came from a single tool-results .txt file under .claude/projects/, where Claude Code spills oversized tool outputs. That file contained a Python traceback with embedded Claude Code session JSONL inlined as the "filename" of an OSError. normalize() returned it verbatim because none of the JSON parsers extracted messages, then chunk_exchanges fell through to paragraph-mode and smart_split hard-cut at ceiling because the JSON blob has no natural-language boundaries. Two-layer fix: (1) Source-set hygiene — palace.SKIP_DIRS gains "tool-results". Claude Code per-session tool-results subdirectories contain raw tool artifacts (JSON dumps, log captures, error tracebacks with inlined session metadata), not conversation content. Skipping the subtree at the walker level prevents the failure mode at the source. (2) Defense-in-depth — chunker.is_excluded_content gains JSON-blob detection. Two new heuristics: - >=50% of non-empty lines start with `{"key":` (the canonical JSONL session-log shape) - High `,"key":` density (>=1 per 200 chars) combined with a transcript marker (uuid/sessionId/requestId/messageId/ parentUuid/timestamp:YYYY-) is decisive Real prose mentioning a uuid or showing one inline JSON example survives — only blob-level density triggers exclusion. Also: convo_miner._emit_chunks and miner.chunk_text now apply is_excluded_content per-chunk in addition to the whole-content check. A largely-prose response with one embedded log/JSON paragraph drops just that chunk rather than the whole exchange. Validation against real production sources: - tool-results/b5k5gj8gj.txt (the actual 82-bad-drawer culprit): whole-file excluded, 0 chunks produced (was 82 mid-line drawers) - real Claude Code .jsonl session: 79K -> 13K transcript via normalize, 11 prose chunks, 0% length-cliff @800 - prose with inline JSON example: kept as one clean chunk Tests: 997 pass (was 991), +5 chunker exclusion cases, +1 walker skip-dir case. Forward-only — existing 95K drawers untouched. The 82 noise drawers from the b5k5gj8gj.txt artifact remain in the palace pending a separate selective-delete decision.
Adds a clean public API for reading an agent's diary entries from the
palace, encapsulating the chromadb where-filter + sort + slice logic
that consumers (Vestige's runtime_orientation, primarily) were
inlining via mempalace.palace.get_collection.
Surface:
- DiaryEntry dataclass (frozen): date, filed_at, topic, content.
Mirrors the persistence shape used by tool_diary_write /
tool_diary_read in mcp_server.
- DiaryUnavailable exception: raised when the palace is unreachable
or the diary collection cannot be queried. Distinguishes
infrastructure failure (palace missing, chromadb error) from a
genuinely empty diary (returns []). Callers who care can render
differently: DiaryUnavailable -> "(diary unavailable)", [] ->
"(no entries yet)".
- read_diary(agent, last_n=5, *, palace_path=None) -> list[DiaryEntry]:
Filters wing=wing_{agent.lower()} room=diary, sorts by filed_at
descending, slices to last_n.
Side-effect-free: uses palace.get_collection (not mcp_server, which
performs dup2(stderr, stdout) at import time as part of the MCP stdio
protocol contract and would clobber consumer log streams).
Consumer migration: Vestige's runtime_orientation moves from inline
chromadb logic to this API in a coordinated PR. The TODO at
runtime_orientation.py:196 was the load-bearing comment that named
this exact API; that comment goes away in the consumer migration.
Tests: 10 new in tests/test_read_diary.py covering happy-path sort/
last_n/empty/wing-filter/case-insensitive-agent, plus DiaryUnavailable
raised on get_collection failure + col.get failure, plus palace_path
override semantics.
10/10 pass. No regression to existing layers tests.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a meaning-aware chunking system to improve memory indexing by preserving document structure and excluding noise, and adds a filed_at_ts numeric timestamp to metadata across all ingestion paths, supported by a new backfill script. It also implements a structured read_diary API. Feedback focuses on resolving portability regressions caused by hardcoded local paths and Python interpreter locations in shell hooks, as well as addressing a redundant import and potential data truncation risks in the diary retrieval logic.
| # Optional: run mempalace ingest synchronously so memories land before compaction | ||
| # Run mempalace ingest synchronously so memories land before compaction. | ||
| # Prefer MEMPAL_DIR if set; otherwise fall back to the active transcript's directory. | ||
| PYTHON="/Users/jameswinans/Development/AI/mempalace/.venv/bin/python" |
Contributor
There was a problem hiding this comment.
The PYTHON path is hardcoded to a local directory on your machine. This will break the hook for any other user. It should be generalized to use the system python3 or be configurable via an environment variable.
Suggested change
| PYTHON="/Users/jameswinans/Development/AI/mempalace/.venv/bin/python" | |
| PYTHON="$(command -v python3)" |
| # 2. MEMPAL_DIR (user-configured) — mine that directory | ||
| # At least one should work. If neither is set, nothing mines. | ||
| PYTHON="$(command -v python3)" | ||
| PYTHON="/Users/jameswinans/Development/AI/mempalace/.venv/bin/python" |
Contributor
There was a problem hiding this comment.
This change replaces a portable command discovery (command -v python3) with a hardcoded local path. This is a regression in portability and will cause the script to fail on other systems.
Suggested change
| PYTHON="/Users/jameswinans/Development/AI/mempalace/.venv/bin/python" | |
| PYTHON="$(command -v python3)" |
Comment on lines
+82
to
+85
| *-Users-jameswinans-Documents-Temenos-Ves/*) AGENT="ves"; WING="ves_sessions" ;; | ||
| *-Users-jameswinans-Documents-Temenos-Kai/*) AGENT="kai"; WING="kai_sessions" ;; | ||
| *-Users-jameswinans-Documents-Temenos-Mira/*) AGENT="mira"; WING="mira_sessions" ;; | ||
| *-Users-jameswinans-Documents-Temenos-Adrian/*) AGENT="adrian"; WING="adrian_sessions" ;; |
Contributor
There was a problem hiding this comment.
These case patterns contain absolute paths specific to your local environment (/Users/jameswinans/...). These should be removed or generalized to patterns that are likely to exist for other users, or the logic should be moved to a configuration file.
Comment on lines
+161
to
+164
| *-Users-jameswinans-Documents-Temenos-Ves/*) AGENT="ves"; WING="ves_sessions" ;; | ||
| *-Users-jameswinans-Documents-Temenos-Kai/*) AGENT="kai"; WING="kai_sessions" ;; | ||
| *-Users-jameswinans-Documents-Temenos-Mira/*) AGENT="mira"; WING="mira_sessions" ;; | ||
| *-Users-jameswinans-Documents-Temenos-Adrian/*) AGENT="adrian"; WING="adrian_sessions" ;; |
Contributor
There was a problem hiding this comment.
The patterns in this case statement are hardcoded to your local machine's directory structure. This logic will not function correctly for other contributors or users of the repository.
Comment on lines
+435
to
+439
| try: | ||
| from .palace import get_collection | ||
| except Exception as exc: | ||
| logger.debug("read_diary: palace module not importable: %s", exc) | ||
| raise DiaryUnavailable("palace module not importable") from exc |
Contributor
There was a problem hiding this comment.
This try...except block and local import are redundant. get_collection is already imported at the module level (line 27) as _get_collection. If the palace module were not importable, the error would have occurred when layers.py was first loaded. You should use the existing _get_collection instead.
# Use the already imported _get_collection
Comment on lines
+460
to
+464
| results = col.get( | ||
| where={"$and": [{"wing": wing}, {"room": "diary"}]}, | ||
| include=["documents", "metadatas"], | ||
| limit=10_000, | ||
| ) |
Contributor
There was a problem hiding this comment.
Using a fixed limit=10_000 without server-side sorting (which ChromaDB's get does not support) creates a risk of returning incorrect results as the diary grows. If a user has more than 10,000 entries, col.get will return an arbitrary subset (usually the first 10k by ID), and the subsequent Python sort will only see those, potentially missing the actual most recent entries. Consider using a filter on filed_at_ts (e.g., for the last 30 days) or fetching the total count and using an offset if insertion order is guaranteed.
Author
|
Mis-targeted: opened against upstream MemPalace/mempalace by gh CLI's default base resolution. Reopening on jpwinans/mempalace main (the fork our editable install actually consumes). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
read_diary(agent, last_n=5, *, palace_path=None) -> list[DiaryEntry]as a public API inmempalace.layers. Encapsulates the chromadb where-filter + sort + slice logic that consumers (Vestige'sruntime_orientation, primarily) were inlining viapalace.get_collection.Public surface
DiaryEntry(frozen dataclass) —date,filed_at,topic,content. Mirrors persistence shape oftool_diary_write/tool_diary_readinmcp_server.DiaryUnavailable(exception) — raised when palace is unreachable or chromadb query fails. Distinguishes infrastructure failure from genuinely empty diary (returns[]). Callers who care:DiaryUnavailable→"(diary unavailable)",[]→"(no entries yet)".read_diary— filterswing=wing_{agent.lower()} room=diary, sorts byfiled_atdescending, slices tolast_n.Why
Consumer (Vestige's
runtime_orientation) had a# TODO: migrate to mempalace.layers.read_diarycomment naming this exact API. Inlining the where-filter + metadata-key knowledge in every consumer means schema changes here cascade across consumers. Centralized inlayers.py, schema knowledge stays in the package that owns the schema.Safety
palace.get_collection(notmcp_server, which performsdup2(stderr, stdout)at import time as part of the MCP stdio protocol contract and would clobber consumer log streams).col.get(...). No writes, no HNSW touch (avoids the chromadb metadata-update pathology we hit on 2026-04-17).Tests
10 new in
tests/test_read_diary.py:filed_atdesclast_nlimit[][]last_n=0returns[]DiaryUnavailableraised onget_collectionfailureDiaryUnavailableraised oncol.getfailurepalace_pathkwarg overrides config10/10 pass. No regression to existing
layers.pytests.Consumer coordination
This PR is the upstream half of a coordinated change. The consumer migration (Vestige's
runtime_orientationcallingread_diaryinstead of inliningget_collection) ships in a separate Vestige PR — that PR depends on this one merging first so its import resolves againstmain. Merge order: mempalace first, then Vestige.Vestige PR: jpwinans/vestige feat/mempalace-read-diary-api (commit e698d51).