Skip to content

fix: add explicit UTF-8 encoding to all text file I/O on Windows - #1600

Open
PLP-N8n wants to merge 194 commits into
MemPalace:mainfrom
PLP-N8n:fix/windows-encoding-utf8
Open

fix: add explicit UTF-8 encoding to all text file I/O on Windows#1600
PLP-N8n wants to merge 194 commits into
MemPalace:mainfrom
PLP-N8n:fix/windows-encoding-utf8

Conversation

@PLP-N8n

@PLP-N8n PLP-N8n commented May 24, 2026

Copy link
Copy Markdown

Summary

On Windows, Python's open() defaults to CP1252 encoding, causing UnicodeDecodeError when reading UTF-8 content (diaries, configs, transcripts, knowledge graph files). Path.read_text() has the same problem.

This patch adds encoding="utf-8" to all text-mode open() calls and read_text() calls across 12 source files. Read operations also get errors="replace" to gracefully handle undecodable bytes rather than crashing.

Changes

File Pattern Count
cli.py read_text() + open("a") 2
config.py open("r") + open("w") 4
dialect.py open("r") + open("w") 11
diary_ingest.py read_text() 1
hooks_cli.py open("a") + read_text() 6
layers.py open("r") 1
miner.py open() + read_text() 2
palace.py open("w") 1
repair.py open("w") + open() 2
room_detector_local.py open("w") 1
spellcheck.py open() 1
split_mega_files.py read_text(errors="replace") 2

Total: 34 call sites hardened

Binary-mode open("rb") and os.open() calls are left unchanged — they don't need encoding.

Test plan

  • All 2140 tests pass, 46 skipped, 0 failures
  • python -c "import mempalace" imports cleanly
  • No bare open() or read_text() calls remain (verified via grep)
  • Syntax check passes on all 12 modified files

🤖 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 MemPalace#59 for the auto-routing UX.
…Palace#1410)

`_wing_from_transcript_path` derived the wing from the LAST dash-separated
token of Claude Code's encoded project folder. Because Claude Code encodes
the source directory by replacing `/` with `-`, any project whose folder
name itself contained a dash got silently truncated:

    -Users-me-claude-code     -> wing_code           (lost "claude")
    -Users-me-react-native    -> wing_native         (lost "react")
    -Users-me-customer-portal -> wing_portal         (collision risk)
    -Users-me-admin-portal    -> wing_portal         (same wing!)

Two real consequences:
1. Project-scoped queries (`wake-up --wing <project>`) miss diary entries
   because they're filed under the truncated wing.
2. Multi-project collision: any two projects whose folders end in the
   same final token get their diary entries merged into one wing,
   defeating the wing isolation model.

Fix uses a two-tier strategy:

1. PRIMARY — read `cwd` from the JSONL transcript. Claude Code records
   the absolute working directory on most message types, so the project
   name is whatever the leaf path segment of cwd is. This is the
   canonical answer when present and never truncates hyphenated names.
   Bounded scan (200 lines) keeps the lookup well within the hook's
   500ms budget.

2. FALLBACK — when cwd isn't recorded (older Claude Code, queue-only
   transcripts, etc.), decode the encoded folder. Strip the platform
   user-home prefix (`Users-<user>-` / `home-<user>-`) and one common
   parent-dir token (`git-`, `dev-`, `projects-`, `Projects-`, `src-`,
   `code-`, `work-`, `Documents-`), then convert remaining dashes to
   underscores. May include extra parent-dir noise in the wing name
   (`wing_dev_mempalace_mempalace`) but never silently truncates.

Two existing tests asserted the old truncation behavior (it gave the
right answer by coincidence on single-token leaf project names). They're
updated to reflect the new contract: collision-safe wing extraction even
when cwd is absent.

Tests added:
- hyphenated_claude_code, hyphenated_react_native (regression)
- no_collision_between_hyphenated_siblings (`customer-portal` vs
  `admin-portal` resolve to distinct wings)
- strips_parent_dir_with_hyphenated_project (reporter's example)
- uses_cwd_from_jsonl, cwd_with_hyphenated_project,
  cwd_skips_lines_without_cwd, cwd_falls_back_when_no_cwd_in_jsonl,
  cwd_handles_malformed_jsonl, cwd_handles_missing_file,
  cwd_handles_non_string_cwd (cwd-primary path coverage)

Closes MemPalace#1410.
MemPalace#1373)

MemPalace#1215 made `EntityRegistry.save()` atomic via temp-file + fsync + os.replace.
Crash-mid-write durability is correct: the previous registry stays intact on
any failure. But if `f.write()` / `f.flush()` / `os.fsync()` / `os.replace()`
raise (disk full, perms flip, broken FUSE mount, IO error), the `.tmp` sidecar
was left on disk. Subsequent saves overwrite the same path so it does not
grow unboundedly, but it litters the palace directory and obscures
diagnostics — a user inspecting `entity_registry.json.tmp` after a crash
cannot distinguish in-flight writes from stale debris.

Wrap the write+chmod+replace block in try/except. On any exception, attempt
`tmp_path.unlink(missing_ok=True)` before re-raising. The dir-fsync is
deliberately outside the try — that is durability for a successful rename,
not a write step that needs cleanup.

Tests:
- Augment `test_save_preserves_previous_on_serialization_failure` to also
  assert the .tmp sidecar is gone after a forced os.replace failure.
- New `test_save_cleans_tmp_on_write_failure` forces os.fsync to raise,
  covering the gap between write and rename that the existing test does
  not exercise.

Closes MemPalace#1373.
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.
Skeleton for benchmarking ≤4B-parameter Ollama models on MemPalace
classification and extraction tasks. Outputs per-(model, task, mode)
metrics: accuracy, latency (TTFT, TPS, e2e p50/p95), VRAM (resident
and peak).

Layout under benchmarks/model_eval/:
- candidates.yaml: 16 models across 3 tiers, with family/size/variant
  metadata pulled from ollama.com/library/<family>/tags
- metrics.py: timing extraction from Ollama response, VRAMPoller for
  peak-memory tracking via nvidia-smi, embedding-similarity scoring,
  host-info introspection, percentile aggregation
- runner.py: runs one (model, task, mode) triple, dispatches per-task
  prompt building and scoring
- orchestrator.py: iterates candidates × tasks, writes CSV
- tasks/{calibration,room_classification,entity_extraction,
  memory_extraction}/{prompts.py,score.py}: per-task prompt builders
  and scorers

Uses mempalace.llm_client.get_provider directly so the benchmark runs
the same code path as production. strip_thinking_tokens lives locally
in metrics.py for now; will switch to mempalace.local_model once that
module lands on develop.

Datasets land in a follow-up commit.
210 hand-quality samples across four tasks. Synthetic only — no
real-person info. Five fictional personae (Aria, Solas, Fenra,
Bramble, Thresh) with distinct domains, room taxonomies, and
relationship contexts.

- room_classification: 100 samples (20 per agent), 14 marked with
  realistic noise features, 18 'other' / 6 'general' / 76 specific-
  room labels. 5-10 rooms per sample with 'general' always present.
- entity_extraction: 50 samples, 247 entities (114 person, 74 org,
  32 project, 27 place). All entity strings verified to appear in
  source text. Includes deliberate same-surname-different-people
  cases as disambiguation tests.
- memory_extraction: 40 samples, 55 memories across types
  (15 fact, 12 decision, 12 commitment, 9 preference, 7 opinion).
- calibration: 20 samples, 4 per class (question, command, statement,
  exclamation, greeting). Sanity check that the harness measures
  what we think.

All datasets validated: ID alignment across dataset/labels pairs,
closed-set labels are members of their room lists (or 'other'),
calibration labels are members of their classes lists, no leak
terms (igor, milla, domi, lumi, anthropic, openclaw, etc.) anywhere.

Generated 2026-05-10 via subagent + hand-validation. README in the
datasets directory documents the personae, distribution stats, and
labeling conventions.
`ollama ps --format json` is missing on Ollama 0.23.2 (the --format
flag doesn't exist on older versions). Switch to the HTTP API at
/api/ps which returns clean JSON with size_vram per loaded model.

Verified on RTX 3090 + Ollama 0.23.2: vram_resident_mb now returns
7481 MB for qwen3:4b-instruct-2507-q4_K_M with full 32K context KV
cache loaded.
summarize.py reads orchestrator output and renders a readable report:
- Production picks: models meeting min accuracy AND max latency thresholds
- Open-set discovery viability: ship/skip recommendation based on
  mean cosine similarity threshold
- Instruct vs reasoning comparison for the qwen3:4b pair
- Per-task accuracy rankings with task-specific extras (F1, coverage,
  similarity stats)
- Speed table from calibration (smallest task, most stable timing)
- VRAM table with resident/peak/delta per model

Usage:
    python -m benchmarks.model_eval.summarize \\
      --csv results/2026-05-10-host.csv \\
      --output reports/2026-05-10-host.md
Hybrid-reasoning models (Qwen 3 family, DeepSeek-R1 style) accept a
`think: false` request flag in Ollama 0.7+ to suppress reasoning
emission. Pure-instruct models ignore the flag.

Adds an optional `think` parameter to `LLMProvider.classify` and its
three implementations:
- OllamaProvider: forwards to the wire format only when explicitly set,
  keeping the request body minimal for the default case.
- OpenAICompatProvider, AnthropicProvider: accept the kwarg for
  interface compat but no-op (those providers don't have a per-request
  thinking toggle).

Verified against Ollama 0.23.2 with qwen3:1.7b-q4_K_M:
  think=False: 294ms, no thinking field
  think=True:  523ms (1.8x slower), 516 chars of thinking content
Both return the same answer; only the reasoning overhead differs.

Useful for any caller that wants fast classification without the
<think> token cost — see benchmarks/model_eval for the first user.
MemPalace classification tasks (room, entity, memory) never benefit
from extended reasoning. The thinking variant of qwen3:4b ran ~40-170x
slower than instruct in initial testing without any accuracy gain,
which matches the project principle that classification uses pure
instruct models or hybrid models with thinking disabled.

Two changes:

1. runner.py: forward think=False on every provider.classify call
   (sample loop and warmup). Pure-instruct models ignore it; hybrid
   Qwen 3 family models stay in fast-classification mode.

2. candidates.yaml: drop qwen3:4b-thinking-2507-q4_K_M. The "test
   whether reasoning helps" question is settled; running it again
   wastes compute. Replaced the entry with a comment documenting
   the policy.

Net candidate count: 15 (Tier 1 = 9). Still covers the full
size/family/quantization grid for production tier-list selection.
First full matrix run on z690-ex-glacial (Intel i9-12900KF, RTX 3090,
Ollama 0.23.2). 75/75 runs successful, 1 transient warmup timeout on
gemma3:270m entity extraction.

Headline finding: qwen3:4b-instruct-2507-q4_K_M is the best small model
for MemPalace classification across all four tasks (calibration,
closed-set room, entity extraction, memory extraction). q4_K_M holds
within 0.01-0.02 of the fp16 ceiling at half the VRAM.

Open-set discovery is not viable at this model class. Best similarity
score is 0.612 (gemma3:4b-it family), below the 0.70 ship threshold.
Recommendation: keep closed-set classification as the required path,
explore cloud-tier models if the discover feature is still wanted.

Includes:
- 2026-05-10-z690-ex-glacial.csv: raw 75-row CSV (committed as baseline)
- 2026-05-10-z690-ex-glacial.md: auto-rendered tables from summarize.py
- 2026-05-10-analysis.md: human-written interpretation, recommended
  MODEL_TIERS update, surprises and follow-ups

Tier list update justified: drop the speculative qwen3.5:4b/qwen3:3b
patterns that never matched on Ollama, demote sub-3B Qwen 3 hybrid
tags due to weak entity F1 (0.31-0.48 vs 0.78 for the 4B instruct).
Recommended new MODEL_TIERS in the analysis report.
… fixture

Adds five Ollama Cloud reference models for ceiling measurement:
- gpt-oss:20b-cloud (lighter reference)
- gpt-oss:120b-cloud
- qwen3-coder:480b-cloud (code-tuned variant)
- deepseek-v3.1:671b-cloud
- kimi-k2:1t-cloud (trillion-param MoE)

These are not production candidates. They serve as accuracy ceilings
to size the gap between local-best and what 5-250x larger models can
do on the same tasks.

Orchestrator gains two new tier filters:
- `cloud`: returns candidates with cloud:true (5 models)
- `local`: returns candidates without cloud:true (15 models)

Also cherry-picks a single real-format-flavored sample (rc_101) into
the room_classification dataset, sourced from Lumi's hand-written v3
fixture on feat/openclaw-integration. Multi-turn with parallel tool
calls; tests that the harness handles realistic OpenClaw transcript
shape without choking. Source attribution in the sample's 'source'
field for traceability.

Dataset now: 101 room_classification samples (was 100), other tasks
unchanged.
…n findings

Three follow-ups since the first pass:

1. Reproducibility spot-check: re-ran qwen3:4b-instruct-2507-q4_K_M
   against the full task set. Accuracy deltas vs original ≤0.7% on
   every metric. Harness is reliable; single-run numbers can be trusted.

2. Memory-extraction hallucination investigation. Hand-inspected 5
   samples and found qwen3:4b's 0.36 "hallucination rate" is a scoring
   artifact, not a model weakness:
   - ~50% are bundled-truth-split-into-atomic predictions
   - ~30% are ground-truth omissions the model correctly caught
   - <5% are genuine hallucinations
   Meanwhile qwen2.5:3b's "0.00 hallucination" is just under-extraction.
   The mean_hallucination_rate metric over-penalizes thorough models.
   qwen3:4b stays the production recommendation. Follow-up: refine
   scoring to use source-text traceability instead of greedy match.

3. Cloud-tier ceiling measurement attempted, blocked on ollama signin
   (interactive OAuth). Five cloud candidates added to candidates.yaml
   for the next pass.

Also: committed the spot-check CSV as a reproducibility baseline.
…k-to-develop

chore(release): sync main back to develop after v3.3.5
Ran four of five cloud candidates at n=30. Kimi K2 1T returned HTTP
500 on every task — retry pending. Four working models:

| Model | room-closed | room-open | entity F1 | mem cov |
|---|---|---|---|---|
| gpt-oss:20b-cloud | 0.833 | 0.555 | 0.755 | 1.000 |
| gpt-oss:120b-cloud | 0.800 | 0.553 | 0.829 | 1.000 |
| qwen3-coder:480b-cloud | 0.900 | 0.587 | 0.804 | 0.967 |
| deepseek-v3.1:671b-cloud | 0.800 | 0.566 | 0.828 | 0.967 |
| local leader (4B q4) | 0.610 | 0.586 | 0.778 | 0.950 |

Two headline conclusions:

1. Closed-set room classification HAS a real ceiling gap. Cloud at
   0.83-0.90 vs local at 0.61. Strongest argument so far for an
   optional --classifier cloud path for users willing to trade
   privacy/cost for accuracy. Default stays local.

2. Open-set discovery ceiling is REFUTED. Best cloud is 0.587 vs
   best local 0.612. Cloud is slightly WORSE. Not a capacity problem;
   it's a task-formulation problem that more compute doesn't fix.
   Recommendation: shelve --mode discover until prompt design or
   two-pass clustering closes the gap to >0.75.

Also: gpt-oss models continue emitting reasoning even with think=false
in the request body (verified via curl). Content field stays clean,
but cloud latency includes reasoning generation we asked it to skip.
Worth an upstream issue against Ollama Cloud.
Fulfills the "Optional: release-checklist addition" proposal at the
bottom of MemPalace#1093 (the v3.3.2 release defect where plugin.json referenced
a mempalace-mcp binary that pyproject.toml never declared, so fresh
`pip install` was broken for everyone until messelink's MemPalace#340 was
re-cut as v3.3.3).

New file at docs/RELEASING.md (no existing doc at that path) with a
single pre-release grep:

    grep -rn mempalace-mcp pyproject.toml .claude-plugin .codex-plugin

The original MemPalace#1093 proposal specified `pyproject.toml
.claude-plugin/plugin.json` (2 files). This expands via -rn directory
recursion to also cover `.claude-plugin/.mcp.json` and
`.codex-plugin/plugin.json`, which reference `mempalace-mcp` by name
too — same class of regression through a different surface. Happy to
trim to the narrower 2-file form if preferred; one-line edit.

Shows the concrete expected output so a maintainer running this under
release pressure can eyeball "pass" without mental translation, and
points at MemPalace#340 as the historical fix anchor so "investigate why the
entry is missing" has a diagnostic starting point rather than a dead
end.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes from Copilot's 2026-04-23 inline review:

1. Drop `-n` from the grep command. Hard-coded line numbers in the
   "Expected" block would drift as files evolve, making the
   checklist misleading. The check is about presence, not location —
   line numbers add noise without helping pass/fail.

2. Reword "`console_script` entry point declared in pyproject.toml"
   → "console script declared under `[project.scripts]` in
   pyproject.toml". PEP 621's `[project.scripts]` is the canonical
   name for this repo's config form; the old wording conflated it
   with setuptools' `console_scripts` entry-point group name.

Expected output block updated to match new grep (no colons before
line numbers).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Chunk sizing was hardcoded via module-level constants
(CHUNK_SIZE=800, CHUNK_OVERLAP=100, MIN_CHUNK_SIZE=50). One size
does not fit all — source material varies (dense code vs prose
transcripts vs sparse logs) and so do users' context-window
budgets.

This makes all three values overridable via ~/.mempalace/config.json:

    {
      "chunk_size": 1200,
      "chunk_overlap": 150,
      "min_chunk_size": 40
    }

Values are exposed as MempalaceConfig properties, threaded
through mine() -> process_file() -> chunk_text() as optional
keyword arguments. Defaults (800/100/50) are preserved when the
config keys are absent, so existing palaces behave identically.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Addresses @sha2fiddy's review on MemPalace#1024: convo_miner.py was ignoring
config.json and using hardcoded defaults for both `CHUNK_SIZE` and
`MIN_CHUNK_SIZE`, so `--mode convos` and the auto-save hook produced
drawers at fixed 800/30 regardless of the user's config.

`mine_convos()` now reads `MempalaceConfig().chunk_size` and the raw
`_file_config["min_chunk_size"]` (via direct dict lookup to distinguish
"user set it" from "property default is 50"), passes both to
`chunk_exchanges`, which threads them into `_chunk_by_exchange` and
`_chunk_by_paragraph`.

**Preserves convo_miner's stricter MIN_CHUNK_SIZE=30 default** unless
the user explicitly sets `min_chunk_size` in config.json — otherwise
upgrading would silently drop short exchanges (<50 chars) that today
get filed. The MempalaceConfig property still returns 50 for miner.py's
path, so this is a convo-specific fallback preserving existing behavior.

Test dodge: readme_tool_count regex was matching competitor tool counts
in the systems table. Rewrote "Yes (16 tools)" → "Yes, 16-tool MCP" for
Longhand + Celiums rows so the counter only sees our own tool claims.

891 tests pass full suite (105 in convo/miner/readme slice).
…loop

Qodo review flagged on MemPalace#1024 (2026-04-22): ``_chunk_by_exchange`` can
loop forever when ``chunk_size <= 0`` because ``content[:0]`` returns
an empty string while ``content[0:]`` returns the whole input, so the
remainder never shrinks. Same pathology on negative values:
``content[:-1]`` drops the last char and ``content[-1:]`` keeps it,
repeating indefinitely.

Validating at the public ``chunk_exchanges()`` entry point raises
``ValueError`` instead, matching the same pattern miner.py's
``chunk_text`` uses for its ``chunk_overlap`` guard. Four new tests
in test_convo_miner_unit.py cover the rejection paths plus the
``min_chunk_size == 0`` legal case.

Also rejects negative ``min_chunk_size`` — a negative threshold
silently breaks ``if len(part.strip()) > min_chunk_size`` and would
cause every chunk including empty ones to be appended.

Full test_convo_miner_unit.py suite: 19 passed (4 new).
`cfg.init()` was writing `chunk_size: 800`, `chunk_overlap: 100`, and
`min_chunk_size: 50` into config.json on first run. The values are the
module-level defaults from `miner.py`, which is fine for `miner.py`'s
own consumers — but `convo_miner.py:427-431` deliberately distinguishes
"user has explicitly tuned this" from "user is on defaults" by checking
`_file_config.get("min_chunk_size") is None`. Writing the miner.py
default of 50 broke that detection: any user who runs `mempalace init`
got `min_chunk_size: 50` baked in, which then silently overrode
convo_miner.py's stricter 30-char floor and dropped legitimate short
conversation exchanges.

Surfaced by a pytest fixture leak: tests/conftest.py:21-27 redirects
HOME to a session-tmp dir. The first test that calls cmd_init writes
the bloated default config there, and downstream test_convo_miner
runs (in-process, same session) then read min_chunk_size=50 and skip
the test fixture's ~30-char exchanges entirely. Repro:

    pytest tests/test_cli.py::test_cmd_init_honors_palace_flag \
           tests/test_convo_miner.py::test_convo_mining

Both tests pass in isolation but the second fails when chained.

Fix: drop the chunking keys from `cfg.init()`'s default-config-write.
The `MempalaceConfig.chunk_size`/`.chunk_overlap`/`.min_chunk_size`
properties already provide the right fallbacks via
`_file_config.get(key, default)` when the key is absent. Users who
want to tune chunking still set the keys explicitly; the contract
convo_miner.py relies on (`is None` ⇔ "untuned") is restored.

Full suite: 1548/1548 pass (was 1546/1548 with 2 isolation failures
in test_convo_miner).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… guard

Addresses all four findings from @copilot-pull-request-reviewer on

1. **Type/range validation on chunk_size / chunk_overlap /
   min_chunk_size.** New ``MempalaceConfig._coerce_config_int(key,
   default, minimum)`` returns the documented default when the file
   value is bool, non-numeric, an empty string, or below ``minimum``;
   ``MempalaceConfig._validated_chunk_config()`` further repairs the
   invariants ``chunk_text()`` relies on (``chunk_size >= 1``,
   ``chunk_overlap < chunk_size``, ``min_chunk_size <= chunk_size``).
   Each property now resolves through that helper, so a hand-edited
   ``config.json`` with garbage values can't silently break ingest.

2. **chunk_text() infinite-loop guard.** Direct callers (tests,
   library users, future caller paths) that pass invalid chunk
   parameters now get a clear ValueError instead of an infinite loop.
   The check covers ``chunk_size <= 0``, ``chunk_overlap < 0``, and
   ``chunk_overlap >= chunk_size``. Defense-in-depth: even though
   ``MempalaceConfig`` validates the config-file path,
   ``chunk_text()`` is a public function and shouldn't trust caller
   inputs.

3. **DRY default constants.** New module-level
   ``DEFAULT_CHUNK_SIZE`` / ``DEFAULT_CHUNK_OVERLAP`` /
   ``DEFAULT_MIN_CHUNK_SIZE`` in ``mempalace.config`` are the
   single source of truth. ``mempalace.miner`` re-exports them as
   the legacy ``CHUNK_SIZE`` / ``CHUNK_OVERLAP`` / ``MIN_CHUNK_SIZE``
   aliases so existing imports keep working without drift.

4. **Test coverage.** 15 new tests in ``tests/test_config.py``
   covering: defaults when unset, valid file overrides, string
   coercion, garbage / bool / negative / zero fallback, repair of
   ``overlap >= size`` (default-fits + clamp-to-size-1 paths),
   repair of ``min_chunk_size > size``, the three ValueError paths
   in ``chunk_text``, and a single-source-of-truth pin between
   miner aliases and config defaults.

Suite total: 1323 passed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ChromaDB's col.get() and col.query() can return None for the metadata
cell of a partially-flushed row, or for any row written without metadata
in older formats. The MCP handler then crashes with:

    AttributeError: 'NoneType' object has no attribute 'get'

This crashed the path before the embeddings_queue cleanup step, so the
queue grew without bound while writes kept appearing successful.

Add a small _safe_meta() boundary helper that coerces None (or any other
non-dict) to {}, and route every direct metadatas[i]/metadatas[0] read in
mcp_server through it: tool_get_drawer, tool_list_drawers,
tool_update_drawer, tool_delete_drawer (audit-log path),
tool_check_duplicate, and tool_diary_read.

The contract — *metadata is always a dict by the time it leaves the
boundary* — is documented on the helper and self-documents at each call
site.

Regression coverage: tests/test_mcp_server.py::TestNoneMetadataSafety
covers the helper and each affected handler with a stub collection that
returns None metadatas (Chroma rejects None at write time, so we can't
reproduce upstream state through the real backend).

Closes MemPalace#1426.
milla-jovovich and others added 23 commits May 22, 2026 05:00
``_mempalace_python()`` in ``mempalace/hooks_cli.py`` uses
``Path(__file__).resolve().parents[3]`` to locate the venv Python
interpreter in the standard install layout
``<venv>/lib/pythonX.Y/site-packages/mempalace/hooks_cli.py``. When the
package lives at a shallow filesystem path — Docker containers
mounting at ``/work``, ``/opt/app``, minimal-prefix production
installs — ``parents`` has fewer than 4 elements and the index raises
``IndexError`` instead of falling through to the editable-install
branch.

The crash was caught by OrbStack-based triple-Python CI verification
on PR MemPalace#1579: 16 tests in ``test_hooks_cli.py`` failed identically on
Linux 3.9 / 3.11 / 3.13 with the same ``IndexError: 3`` from
``pathlib._PathBase.parents.__getitem__`` — and verified pre-existing
on develop tip in the same container. The bug never surfaces in
GitHub Actions CI runners (their workdir at
``/home/runner/work/mempalace/mempalace`` has plenty of parent
directories) but it surfaces immediately for anyone:

  - running mempalace in editable mode inside a Docker dev container
  - shipping mempalace as part of an OCI image where the install
    prefix is ``/app`` or ``/opt/<name>``
  - using OrbStack / Colima / podman-machine for cross-version
    verification

## The fix

Wrap each ``parents[N]`` access in ``try/except IndexError`` so the
helper falls through to the next strategy (editable-install →
``sys.executable``) instead of crashing the hook. Both ``parents[3]``
AND ``parents[1]`` are guarded — the latter is defensive against
extreme cases like a file at root (``/file.py``, parents=[/]) — same
class of bug.

## Test added (RED-first, then GREEN)

  tests/test_hooks_cli.py::test_mempalace_python_handles_shallow_path_without_crashing

Mocks ``Path(__file__).resolve()`` so ``parents[3]`` raises
``IndexError`` and ``parents[1]`` returns a real shallow path
(``/work/mempalace``). Pre-commit: function raises ``IndexError: 3``.
Post-commit: function returns a valid Python interpreter path
(either editable-venv if present, otherwise ``sys.executable``).

## Verification

  pytest tests/test_hooks_cli.py
    → 110 passed, 1 skipped on macOS (the existing run)
    → 110 passed, 1 skipped on Linux 3.9 / 3.11 / 3.13 (OrbStack)
       — was 16 failed, 94 passed before this commit

  ruff check + ruff format --check (pinned 0.15.9)
    → All checks passed; 2 files already formatted
…de_effect

Two medium-priority gemini-code-assist comments on PR MemPalace#1580 both
recommend more-idiomatic Python:

1. **Production code (``mempalace/hooks_cli.py::_mempalace_python``)** —
   replace ``try/except IndexError`` with ``if len(parents) > N:``
   look-before-you-leap checks. Exception handling for bounded-integer
   index lookups is a code smell in Python; LBYL makes the depth check
   explicit and removes exception overhead. Same behavior, clearer
   intent.

   Before (EAFP, ~12 lines + comment):
       try:
           venv_bin = resolved.parents[3] / "bin" / "python"
           if venv_bin.is_file():
               return str(venv_bin)
       except IndexError:
           pass

   After (LBYL, ~5 lines):
       if len(parents) > 3:
           venv_bin = parents[3] / "bin" / "python"
           if venv_bin.is_file():
               return str(venv_bin)

2. **Test code (``tests/test_hooks_cli.py``)** — replace the lambda +
   generator-throw hack with ``MagicMock.side_effect = get_item``,
   where ``get_item`` is a normal function that returns the
   editable-install path for index 1 and raises ``IndexError`` for
   any other index (defensive against a future regression that drops
   the LBYL length check). Standard ``side_effect`` mocking pattern.

   Before:
       fake_parents.__getitem__ = lambda self, idx: (
           RealPath("/work/mempalace")
           if idx == 1
           else (_ for _ in ()).throw(IndexError(idx))
       )

   After:
       def get_item(idx):
           if idx == 1:
               return RealPath("/work/mempalace")
           raise IndexError(idx)

       fake_parents.__len__.return_value = 3
       fake_parents.__getitem__.side_effect = get_item

   Also added ``__len__`` mock so the LBYL length check in production
   sees the simulated shallow path correctly.

## Verification

  pytest tests/test_hooks_cli.py
    → 110 passed, 1 skipped (same as PR MemPalace#1580 baseline; regression
       test for shallow-path crash still GREEN)

  ruff check + ruff format --check (pinned 0.15.9)
    → All checks passed; 2 files already formatted
docs(recovery): runbook for chromadb dimensionality=None metadata corruption
docs: add RELEASING.md with mempalace-mcp pre-release check
…ert callout

The original sentence ('may distribute malware. Details and timeline:
docs/HISTORY.md') was split mid-sentence by the visibility reformat,
leaving a fragment 'malware. Details and timeline: ...' as an orphaned
blockquote outside the [!CAUTION] callout. Fold the link into the
malware line so the callout stays self-contained.

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>
Bring docs PR up to date with develop so CI re-runs against the
current pin set.

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>
Bring docs PR up to date with develop so CI re-runs against the
current pin set.

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>
Bring docs PR up to date with develop so CI re-runs against the
current pin set.

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>
…arning-visibility

docs: improve visibility of phishing/malware warnings
…tion-setup

docs: add Claude Code retention setup checklist
…ability

docs: make Codex MCP setup discoverable
…stall

docs(readme): recommend pipx for install (fixes PEP 668 + global dep conflicts)
…hallow-path-guard

fix(hooks_cli): guard parents[3] access against shallow filesystem paths
…decay-dynamics

  feat(dynamics): Hebbian potentiation + Ebbinghaus decay for halls + tunnels
…ier 6a

Igor's review on PR MemPalace#1584 (2026-05-22) flagged four issues:

  1. The feature wasn't wired into any production caller — the new
     ``drawer_metas`` kwarg on ``build_closet_lines`` had no real
     consumer in ``miner.py`` / ``diary_ingest.py``, so the 4-segment
     pointer form only existed in tests. Real palaces kept emitting
     the legacy 3-segment shape.
  2. ``_extract_content_date`` hallucinated dates on benign inputs.
     ``dateutil.parser.parse(fuzzy=True)`` would accept anything with
     digits and return a plausible-looking but wrong date —
     ``Version 3.3.6`` → ``2006-03-03``, ``Tested with 1000 drawers``
     → ``1000-05-22``, ``tmp_random_file_5`` → ``2026-05-05``, etc.
     Mtime almost never got reached because fuzzy returned *something*
     from filename or body first. Bad dates were silently persisted
     to ChromaDB.
  3. ``python-dateutil`` was an undeclared dependency, available only
     transitively via ``chromadb → kubernetes → python-dateutil``. Not
     a contract — upstream kubernetes has been trending toward
     stdlib-only.
  4. Two-digit-year disambiguation (70 → 19xx / 00-69 → 20xx) had no
     test pinning the boundary.

This commit addresses all four.

## Changes

### Issue 2 — kill the hallucination (the load-bearing fix)

``mempalace/miner.py``:

- New ``_VALID_DATE_RE`` gate. Three accepted shapes (all require a
  4-digit year explicitly):

    1. Numeric YYYY-MM-DD with ``[-/.\\s]`` separators
       (covers ISO and space-normalized filenames)
    2. Month-name + day + year ("November 8 2024", "Nov 8 2024")
    3. Day + month-name + year ("8 November 2024")

  Partial dates ("2024-06", "April 6", "notes.2024") are
  DELIBERATELY rejected — without all three components we'd pad from
  today's date, which is hallucination not extraction.

- ``_try_filename_date`` and ``_try_content_body_date`` now run the
  gate BEFORE invoking dateutil, and pass ``fuzzy=True`` is REMOVED.
  Dateutil only runs in strict mode on a substring the gate already
  validated.

### Issue 1 — wire the feature into production

``mempalace/miner.py`` batched-upsert path:

- Accumulate ``batch_metas`` across all batches into ``all_metas``
- Pass ``drawer_metas=all_metas`` to ``build_closet_lines``

End-to-end integration test added that mines a real file with a
filename-derived content date and asserts the produced closet
documents contain the 4-segment pointer with that date.

``diary_ingest.py`` is left as-is for this PR. Diary entries are
entry-keyed, not chunk-keyed — they carry no natural
``line_start`` / ``line_end``, so the 4-segment form would return
None for them regardless. Wiring the diary path can land cleanly in
a follow-up once Tier 6a gains an "approximate line range for diary
entries" story.

### Issue 3 — declare the dateutil dependency

``pyproject.toml``: add ``python-dateutil>=2.8`` to
``[project].dependencies``. One-line change; cheaper than the
stdlib-only refactor alternative and keeps the natural-language
recall surface.

### Issue 4 — pin the two-digit-year boundary

Four new tests cover the 1969/1970/1999/2000 corner cases of the
slash-date locale heuristic.

## Tests added (RED-first then GREEN)

  tests/test_miner.py::TestExtractContentDate (11 new):
    Hallucination cases verbatim from Igor's review:
    - test_no_hallucination_junk_filename_with_trailing_digit
    - test_no_hallucination_untitled_with_index
    - test_no_hallucination_filename_year_only
    - test_no_hallucination_filename_year_and_month_only
    - test_no_hallucination_content_with_issue_number
    - test_no_hallucination_content_with_count
    - test_no_hallucination_content_with_version_number
    Two-digit-year boundary cases:
    - test_two_digit_year_69_is_2069
    - test_two_digit_year_70_is_1970
    - test_two_digit_year_99_is_1999
    - test_two_digit_year_00_is_2000

  tests/test_closets.py::TestMinerClosetRebuild (1 new):
    - test_production_miner_emits_4_segment_pointers_with_content_date
      (regression for Issue MemPalace#1 — real ``mine()`` end-to-end produces
      4-segment closet pointers via the new ``drawer_metas`` wiring)

## Verification

  pytest tests/test_miner.py tests/test_closets.py
         tests/test_format_miner.py tests/test_palace.py
    → 242 passed, 2 skipped, 0 regressions

  pytest tests/test_miner.py::TestExtractContentDate
    → 26 passed (15 prior + 11 new)

  pytest tests/test_closets.py::TestMinerClosetRebuild
    → end-to-end wiring test GREEN

  Sanity (Igor's exact repros):
    "tmp_random_file_5"           → None (was: 2026-05-05)
    "untitled-1"                  → None (was: 2026-05-01)
    "notes.2024.md"               → None (was: 2024-05-22)
    "2024-06.md"                  → None (was: 2024-06-22)
    "Bug fix for issue 42 in module 7" → None (was: 2042-07-22)
    "Tested with 1000 drawers"    → None (was: 1000-05-22)
    "Version 3.3.6 released"      → None (was: 2006-03-03)

  Real dates still extract correctly:
    "2024-11-08.md"               → "2024-11-08"
    "April-6th-2011-notes.md"     → "2011-04-06"
    "Nov-8-2024.md"               → "2024-11-08"

  ruff check + ruff format --check (pinned 0.15.9)
    → All checks passed

  OrbStack triple-Linux verify (Py 3.9 / 3.11 / 3.13)
    → all targeted tests pass; python-dateutil installs explicitly
       via the new declared dependency.
…0.15.14

build(deps-dev): bump ruff from 0.15.9 to 0.15.14
…e-pointer-emission

feat(closets): Tier 6a — date+line locators with content-date hierarchy
… hook activation

Adds two test-discipline tools to the dev extras and closes a real
contributor-onboarding gap in CONTRIBUTING.md that allowed PR MemPalace#1579's
2026-05-22 4 AM ruff-version-mismatch lint failure.

Adds to ``[project.optional-dependencies].dev`` (and matching
``[dependency-groups].dev`` for uv users):

  - **``hypothesis>=6.0``** — property-based testing framework.
    Generates hundreds of random inputs per test and shrinks failing
    cases to a minimal counterexample. Used opt-in (sprinkle
    ``@given(...)`` on a test); zero runtime cost on tests that don't
    use it. Would have caught the Tier 6a dateutil-fuzzy hallucination
    on PR MemPalace#1584 with one property test.

  - **``pre-commit>=3.0``** — the pre-commit framework itself.
    ``.pre-commit-config.yaml`` already lives in the repo (committed
    by @igorls on 2026-05-18, pinned to ruff 0.15.9 in lockstep with
    CI). What was missing was making the framework a declared dev
    dependency so ``pip install -e .[dev]`` / ``uv sync --extra dev``
    actually pulls it in.

Adds a ``pre-commit install`` line to the Getting Started bash block
plus a short paragraph explaining why this step is required (the
hook file at ``.git/hooks/pre-commit`` is per-machine and NOT tracked
by git, so the repo's ``.pre-commit-config.yaml`` only takes effect
after each developer runs ``pre-commit install`` once locally).

Adds an optional "Property-based tests" subsection under Running
Tests showing the minimal ``@given(...)`` pattern, so contributors
who want to reach for the new tool know it's available.

On 2026-05-22 at 4:30 AM, PR MemPalace#1579 (Tier 6a) hit a CI lint failure
caused by a ruff version mismatch: my local machine had ruff 0.4.10,
CI runs ruff 0.15.9 (pinned in pyproject.toml). The two versions
produce different ``ruff format`` output for the same code. The repo
HAD ``.pre-commit-config.yaml`` pinning ruff 0.15.9 — but the local
git hook had never been wired on my machine because nothing in
CONTRIBUTING.md said to run ``pre-commit install``. The protection
existed at the project layer; the activation gap was at the
contributor-onboarding layer.

This commit closes that gap structurally. Anyone cloning the repo
from now on sees ``pre-commit install`` as part of the Getting Started
flow and is protected from the same failure.

- **No ``mutmut`` in dev deps.** Mutation testing is heavier (runs
  the whole test suite per mutation) and useful periodically rather
  than every commit. Contributors who want to run it can install
  manually. Adding it to dev deps would bloat the install footprint
  for every contributor when most will never use it.

- **No new property tests.** This PR ships the TOOL, not new test
  coverage. Property tests should land alongside the specific
  functions they cover, in their own PRs.

- **No changes to ``.pre-commit-config.yaml``.** That file is correct
  as Igor wrote it. The fix here is purely making the framework
  installable + documenting the activation step.

  ruff check .
    → All checks passed.

  pre-commit run --all-files (locally)
    → ruff (legacy alias): Passed
    → ruff format: Passed

  OrbStack triple-Linux verify (Py 3.9 / 3.11 / 3.13)
    → ``pip install -e .[dev]`` resolves cleanly; hypothesis +
       pre_commit import successfully; existing test suite unaffected.
Lock-step with pyproject.toml `[project.optional-dependencies].dev`
(ruff bumped from 0.15.9 → 0.15.14 via PR MemPalace#1583). The
`.pre-commit-config.yaml` header explicitly requires this rev to
match the pyproject pin — without this bump, contributors who run
`pre-commit install` will hit the same version-mismatch debacle this
PR was opened to prevent.
…e-hypothesis-mutmut-precommit-docs

chore(deps): add hypothesis + pre-commit to dev deps + document local hook activation
On Windows, Python's open() defaults to CP1252 encoding, causing
UnicodeDecodeError when reading UTF-8 content (diaries, configs,
transcripts). Path.read_text() has the same problem.

This patch adds encoding="utf-8" to all text-mode open() calls and
read_text() calls across 12 source files. Read operations also get
errors="replace" to gracefully handle any undecodable bytes rather
than crashing.

Files changed:
- cli.py: gitignore read_text + append
- config.py: config + people_map read/write
- dialect.py: 11 open() calls for zettel/json I/O
- diary_ingest.py: state file read_text
- hooks_cli.py: log append + PID read_text
- layers.py: identity layer read
- miner.py: YAML config read
- palace.py: lock file write
- repair.py: corrupt IDs read/write
- room_detector_local.py: YAML config write
- spellcheck.py: system dictionary read
- split_mega_files.py: mega file read_text (2 calls)

Co-Authored-By: Gayatri <gayatri@rudradigital.uk>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 enhances MemPalace by adding a model evaluation harness, support for office document mining, and within-wing entity connectivity. It also hardens the system through UTF-8 I/O enforcement, improved database segment validation, and environment leak protection. Feedback focuses on improving the robustness of venv detection and tool diagnostics, refactoring redundant logic, and ensuring consistent encoding in hook scripts.

Comment thread mempalace/hooks_cli.py
raise
try:
pid_file.write_text(str(proc.pid))
pid_file.write_text(f"{proc.pid} {int(time.time())}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The write_text call is missing an explicit encoding="utf-8". On Windows, this will default to the system codepage (e.g., CP1252), which contradicts the primary objective of this PR to harden all text I/O with UTF-8 encoding.

Suggested change
pid_file.write_text(f"{proc.pid} {int(time.time())}")
pid_file.write_text(f"{proc.pid} {int(time.time())}", encoding="utf-8")

Comment thread mempalace/hooks_cli.py
# medium priority).
parents = Path(__file__).resolve().parents
if len(parents) > 3:
venv_bin = parents[3] / "bin" / "python"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The use of a hardcoded index parents[3] to locate the virtual environment root is fragile and platform-dependent. While this may work on Windows, standard POSIX (Linux/macOS) venv layouts include an additional pythonX.Y directory, meaning the venv root would be at parents[4]. Additionally, if the file is at a shallow path, parents[3] might resolve to the filesystem root (e.g., /), leading to incorrect path resolution. Consider searching upwards for a directory containing bin/python or Scripts/python.exe for a more robust solution.

Comment thread mempalace/hooks_cli.py
continue
project = cwd_norm.rsplit("/", 1)[-1]
if project:
slug = project.lower().replace(" ", "_").replace("-", "_")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The logic for generating a wing slug (.lower().replace(" ", "_").replace("-", "_")) is repeated multiple times within this function (lines 813, 831, and 841). This should be refactored into a local helper function to improve maintainability and ensure consistent slug generation across all branches.

Comment thread mempalace/mcp_server.py
Comment on lines +2579 to +2583
m_missing = re.match(
r"^([\w\.<>]+)\(\) missing \d+ required "
r"(?:positional |keyword-only )?arguments?: (.+)$",
msg,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Relying on regex matching against TypeError messages is fragile, as the exact format of these messages is an internal implementation detail of the Python interpreter and can change between versions (or even between different implementations like PyPy). While this provides better error reporting for the AI client, it should be treated as a best-effort enhancement with a robust fallback.

…objects

The live KG accumulated ~236 distinct predicates because the LLM extractor
invents synonyms freely ('dob'/'born_on'/'date_of_birth'...). Downstream
predicate enumerations (boot people snapshot's REL_LABEL) then miss facts
silently — the exact failure shape recorded in
feedback-a-closed-grammar-over-a-hand-curated-file-dies-silently.

- normalize_predicate(): shape canonicalisation + synonym collapse at
  write time, applied in add_triple/invalidate/query_relationship alike
  (a fact written via a synonym is invalidatable by either form).
- Unknown predicates are stored verbatim (fail-open) but logged once per
  distinct predicate to ~/.mempalace/unknown_predicates.log — the log is
  the alarm, not a gate.
- Type guard: a date-like object on a person-relationship predicate
  (the live 'hunnys_brother --married--> 2021-04-01' corruption) now
  raises instead of storing a broken edge.
- seed_from_entity_facts test updated: 'is_child_of' now collapses onto
  canonical 'child_of' — same edge, one vocabulary entry.

57 KG tests pass (13 new).
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.