feat: configurable chunk_size, chunk_overlap, min_chunk_size - #1024
Conversation
Both filed against upstream/develop today: - MemPalace#1023 — PID file guard prevents stacking mine processes - MemPalace#1024 — configurable chunk_size / chunk_overlap / min_chunk_size Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR makes the miner’s chunking behavior user-configurable via ~/.mempalace/config.json, while keeping the current hardcoded defaults as the effective behavior when no overrides are provided.
Changes:
- Adds
chunk_size,chunk_overlap, andmin_chunk_sizeoptional parameters tochunk_text()andprocess_file(), and threads them throughmine(). - Adds
MempalaceConfigproperties forchunk_size,chunk_overlap, andmin_chunk_size(with default values matching the prior constants).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
mempalace/miner.py |
Threads configurable chunk sizing through mine() → process_file() → chunk_text() and applies the parameters in chunking logic. |
mempalace/config.py |
Exposes three new config properties reading chunk_* values from config.json with defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @property | ||
| def chunk_size(self): | ||
| """Characters per drawer chunk.""" | ||
| return self._file_config.get("chunk_size", 800) | ||
|
|
||
| @property | ||
| def chunk_overlap(self): | ||
| """Overlap between adjacent chunks.""" | ||
| return self._file_config.get("chunk_overlap", 100) | ||
|
|
||
| @property | ||
| def min_chunk_size(self): | ||
| """Minimum chunk size — skip smaller chunks.""" | ||
| return self._file_config.get("min_chunk_size", 50) |
There was a problem hiding this comment.
These config properties return raw JSON values without type/range validation. A non-int (e.g. string) or out-of-range value (negative, min_chunk_size > chunk_size, etc.) will currently crash mining or produce 0 drawers unexpectedly. Consider coercing to int where possible and falling back to defaults (or raising a clear ValueError) when values are invalid, and enforce invariants like min_chunk_size <= chunk_size and 0 <= chunk_overlap < chunk_size.
| @property | |
| def chunk_size(self): | |
| """Characters per drawer chunk.""" | |
| return self._file_config.get("chunk_size", 800) | |
| @property | |
| def chunk_overlap(self): | |
| """Overlap between adjacent chunks.""" | |
| return self._file_config.get("chunk_overlap", 100) | |
| @property | |
| def min_chunk_size(self): | |
| """Minimum chunk size — skip smaller chunks.""" | |
| return self._file_config.get("min_chunk_size", 50) | |
| def _coerce_config_int(self, key, default, minimum=None): | |
| """Return a validated integer config value or the provided default.""" | |
| value = self._file_config.get(key, default) | |
| if isinstance(value, bool): | |
| return default | |
| try: | |
| if isinstance(value, str): | |
| value = value.strip() | |
| if not value: | |
| return default | |
| value = int(value) | |
| except (TypeError, ValueError): | |
| return default | |
| if minimum is not None and value < minimum: | |
| return default | |
| return value | |
| def _validated_chunk_config(self): | |
| """Return validated chunk configuration values.""" | |
| default_chunk_size = 800 | |
| default_chunk_overlap = 100 | |
| default_min_chunk_size = 50 | |
| chunk_size = self._coerce_config_int("chunk_size", default_chunk_size, minimum=1) | |
| chunk_overlap = self._coerce_config_int("chunk_overlap", default_chunk_overlap, minimum=0) | |
| min_chunk_size = self._coerce_config_int("min_chunk_size", default_min_chunk_size, minimum=0) | |
| if chunk_overlap >= chunk_size: | |
| chunk_overlap = ( | |
| default_chunk_overlap | |
| if default_chunk_overlap < chunk_size | |
| else max(0, chunk_size - 1) | |
| ) | |
| if min_chunk_size > chunk_size: | |
| min_chunk_size = ( | |
| default_min_chunk_size | |
| if default_min_chunk_size <= chunk_size | |
| else chunk_size | |
| ) | |
| return chunk_size, chunk_overlap, min_chunk_size | |
| @property | |
| def chunk_size(self): | |
| """Characters per drawer chunk.""" | |
| return self._validated_chunk_config()[0] | |
| @property | |
| def chunk_overlap(self): | |
| """Overlap between adjacent chunks.""" | |
| return self._validated_chunk_config()[1] | |
| @property | |
| def min_chunk_size(self): | |
| """Minimum chunk size — skip smaller chunks.""" | |
| return self._validated_chunk_config()[2] |
There was a problem hiding this comment.
Done in 507bcb1. New MempalaceConfig._coerce_config_int(key, default, minimum) and _validated_chunk_config() helpers route every chunk_* property read through type/range validation: bool, non-numeric strings, empty strings, and below-minimum values fall back to the documented default. _validated_chunk_config then enforces the invariants chunk_text() relies on — chunk_size >= 1, chunk_overlap < chunk_size (with the default-fits-vs-clamp-to-size-minus-1 repair you suggested), and min_chunk_size <= chunk_size. Repairs rather than raises so a single bad config.json key can't take ingest down.
| @property | ||
| def chunk_size(self): | ||
| """Characters per drawer chunk.""" | ||
| return self._file_config.get("chunk_size", 800) | ||
|
|
||
| @property | ||
| def chunk_overlap(self): | ||
| """Overlap between adjacent chunks.""" | ||
| return self._file_config.get("chunk_overlap", 100) | ||
|
|
||
| @property | ||
| def min_chunk_size(self): | ||
| """Minimum chunk size — skip smaller chunks.""" | ||
| return self._file_config.get("min_chunk_size", 50) |
There was a problem hiding this comment.
New public config surface (chunk_size, chunk_overlap, min_chunk_size) isn’t covered by existing config tests. Since there are already tests/test_config.py cases for config defaults and file overrides, adding assertions for these new properties (default values and file overrides, plus a couple invalid-value fallbacks if validation is added) would help prevent regressions.
There was a problem hiding this comment.
Done in 507bcb1 — 15 new tests under "Chunk-config validation" in tests/test_config.py. Coverage: defaults-when-unset, valid file overrides, string coercion, garbage/bool/negative/zero fallback, repair of overlap >= size (both the default-fits and clamp-to-size-1 paths), repair of min_chunk_size > size, the three ValueError paths in chunk_text, plus a single-source-of-truth pin between the legacy miner aliases and the new config defaults. 42/42 test_config pass; 1323/1323 full suite.
| @@ -370,20 +385,20 @@ def chunk_text(content: str, source_file: str) -> list: | |||
| chunk_index = 0 | |||
|
|
|||
| while start < len(content): | |||
| end = min(start + CHUNK_SIZE, len(content)) | |||
| end = min(start + chunk_size, len(content)) | |||
|
|
|||
| # Try to break at paragraph boundary | |||
| if end < len(content): | |||
| newline_pos = content.rfind("\n\n", start, end) | |||
| if newline_pos > start + CHUNK_SIZE // 2: | |||
| if newline_pos > start + chunk_size // 2: | |||
| end = newline_pos | |||
| else: | |||
| newline_pos = content.rfind("\n", start, end) | |||
| if newline_pos > start + CHUNK_SIZE // 2: | |||
| if newline_pos > start + chunk_size // 2: | |||
| end = newline_pos | |||
|
|
|||
| chunk = content[start:end].strip() | |||
| if len(chunk) >= MIN_CHUNK_SIZE: | |||
| if len(chunk) >= min_chunk_size: | |||
| chunks.append( | |||
| { | |||
| "content": chunk, | |||
| @@ -392,7 +407,7 @@ def chunk_text(content: str, source_file: str) -> list: | |||
| ) | |||
| chunk_index += 1 | |||
|
|
|||
| start = end - CHUNK_OVERLAP if end < len(content) else end | |||
| start = end - chunk_overlap if end < len(content) else end | |||
There was a problem hiding this comment.
chunk_text() can enter an infinite loop (or behave unpredictably) if chunk_overlap >= chunk_size or if either value is non-positive. Since these values are now user-configurable, add explicit validation (e.g., require chunk_size > 0 and 0 <= chunk_overlap < chunk_size, and guard min_chunk_size bounds) and fail fast with a clear error or fall back to safe defaults so mining can't hang.
There was a problem hiding this comment.
Done in 507bcb1. chunk_text() now starts with explicit invariant checks: chunk_size <= 0, chunk_overlap < 0, and chunk_overlap >= chunk_size each raise ValueError with a message identifying which knob is bad and why (the chunk_overlap >= chunk_size message specifically calls out the infinite-loop hazard). Defense in depth: MempalaceConfig._validated_chunk_config repairs the config-file path, but chunk_text is a public function and shouldn't trust caller inputs either. Three regression tests in tests/test_config.py (test_chunk_text_rejects_*).
| @property | ||
| def chunk_size(self): | ||
| """Characters per drawer chunk.""" | ||
| return self._file_config.get("chunk_size", 800) | ||
|
|
||
| @property | ||
| def chunk_overlap(self): | ||
| """Overlap between adjacent chunks.""" | ||
| return self._file_config.get("chunk_overlap", 100) | ||
|
|
There was a problem hiding this comment.
The default values in MempalaceConfig.chunk_size/chunk_overlap/min_chunk_size are hardcoded literals (800/100/50), duplicating the constants in miner.py. This can drift if the module defaults ever change; consider defining shared DEFAULT_CHUNK_* constants (or importing the miner constants) so there is a single source of truth for defaults.
There was a problem hiding this comment.
Done in 507bcb1. Added DEFAULT_CHUNK_SIZE, DEFAULT_CHUNK_OVERLAP, DEFAULT_MIN_CHUNK_SIZE at module scope in mempalace.config (single source of truth, kept here rather than miner.py to avoid circular imports). mempalace.miner re-exports them as the legacy CHUNK_SIZE / CHUNK_OVERLAP / MIN_CHUNK_SIZE aliases so existing imports of those names keep working. test_miner_constants_alias_config_defaults pins the equivalence so a future drift would surface as a test failure.
|
Same defaults are hardcoded in On
Suggest plumbing Note the |
…#1023 merged, MemPalace#673 rebased - Bump fork-ahead section header to "after v3.3.2" - Strike #11 (quarantine_stale_hnsw) and #18 (PID file guard) as merged-into-upstream-via-v3.3.2, keep entries for traceability - Add v3.3.2-shipped items to "Merged into upstream (post-v3.3.1)" - Rebuild PR table: 10 merged / 7 open / 7 closed; add MemPalace#1024 row, reclassify MemPalace#681/MemPalace#1000/MemPalace#1023 as merged, note MemPalace#673 rebased 2026-04-21 - Annotate MemPalace#661 status with the GitHub review-state machine caveat (CHANGES_REQUESTED persists until reviewer dismisses, not owed) - Bump test count 1063 → 1096 post-merge
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).
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).
|
Fixed in 9e95456 — thanks again for the careful read.
Full test suite still green (891 passed, 106 deselected benchmarks). Let me know if you'd prefer a different fallback strategy — e.g. aligning both miners to 50 as the unified default — happy to iterate. |
…e we already have a PR that addresses them Sweep caught three adjacent issues I hadn't cross-referenced: - MemPalace#854 (silent_save flag never read) — closed by MemPalace#673; added to row + PR body - MemPalace#848 (remove drawers from a wing) — closed by MemPalace#1087; added to row + PR body - #390 (default chunk exceeds MiniLM token cap) — addressed by MemPalace#1024 (configurable unblocks the user without changing default) Posted explanatory comments on all three issues pointing users to the relevant PRs. PR bodies updated via REST API (gh pr edit's GraphQL path was failing on a deprecation warning around projectCards).
|
Hi, Conversation exchange chunking can hang forever when chunk_size <= 0 because the remainder slicing loop never shrinks the string. Severity: action required | Category: reliability How to fix: Validate chunk_size > 0 Agent prompt to fix - you can give this to your LLM of choice:
Found by Qodo code review |
…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).
|
@qodo-ai-reviewer thanks — fixed in Same guard shape as |
…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).
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).
ee5dd91 to
3c16739
Compare
…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).
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).
3c16739 to
9aa0a14
Compare
…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).
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).
9aa0a14 to
3da65ef
Compare
|
@igorls — same v3.3.5 ask as I posted on #1378. This one's been ready since 2026-04-19 (qodo-acked, all 6 CI green). Addresses an actively-cited bug class — the |
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).
b770dff to
c88dce3
Compare
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>
c88dce3 to
b1d75b3
Compare
The MemPalace#1024 merge added `import pytest` to tests/test_convo_miner_unit.py while a pytest import already existed from another merged change. The collision was textually clean so git didn't flag it, but CI ruff reports F811 (redefinition of unused `pytest`), turning lint red on develop and every branch cut from it. Drop the duplicate import. `ruff check .` (0.4.x, CI pin) clean; test_convo_miner_unit 23 passed.
…fig reach MemPalace#1024 routed min_chunk_size for the project miner through the repairing MempalaceConfig validation, but the convo path read the raw, unvalidated _file_config["min_chunk_size"]. A garbage/negative/bool config value would TypeError the convo length gate or ValueError out of chunk_exchanges and abort the whole convo mine — breaking the PR's own "a bad config.json key never takes ingest down" guarantee for one path only. Extract _try_coerce_int helper (shared with _coerce_config_int) and add a public MempalaceConfig.min_chunk_size_explicit accessor that returns the validated value when the user set it, else None. convo_miner uses the None sentinel to keep its stricter 30-char floor for untuned users while honoring a valid override, with no raw _file_config reach. Adds accessor + convo-fallback regression tests.
…-13) 60 develop commits merged into fork main; last sync 2026-05-13 via PR MemPalace#1487. Conflict resolutions (12 files, 15 hunks): - .claude-plugin/hooks/hooks.json — kept fork's palace-daemon clients/hook.py routing (post-2026-05-11 split-brain fix). - mempalace/cli.py — adopted upstream's `_open_collection_or_explain` state-aware helper (MemPalace#1498). - mempalace/convo_miner.py — adopted upstream's `min_chunk_size_explicit` validated accessor (MemPalace#1024 review fix), preserved fork's `hook_verbatim_mode` knob. - mempalace/hooks_cli.py — adopted upstream's `_wing_from_jsonl_cwd` + hyphenated-project-aware fallback (MemPalace#1410, fixes wing collisions for ``claude-code``/``react-native``/``customer-portal``). Added TODO: post-merge reconcile — fork's hooks route through palace-daemon's own wing derivation, so this in-process path is no longer the production codepath on the fork. Verify daemon-side wing convention matches the new ``wing_<project>`` shape. - mempalace/mcp_server.py — adopted upstream's `_canonicalize_kg_path` helper (realpath + normcase, 13-commit canonicalization series: 4dc3ccf/51eb279/3ee158e/6f9fe80) AND preserved fork-only AGE-backend routing branch in `_get_kg`. Combined fork's #49 daemon-vs-local routing log with upstream's `_maybe_eager_warmup_embedder` (MemPalace#1495) in the startup hook (warmup is local-only, skipped in daemon-strict mode). Threaded `_safe_meta` through diary-read iteration. - mempalace/miner.py — adopted upstream's `chunk_size`/`chunk_overlap`/ `min_chunk_size` parameter pass-through + DEFAULT_CHUNK_* re-export (MemPalace#1024) and preserved fork's `symbol_header_prefix` callable + `CHROMA_BATCH_LIMIT` alias. - mempalace/palace.py — combined imports from both sides (PalaceRef/get_backend/resolve_backend_for_palace from fork, BackendClosedError/CollectionNotInitializedError/PalaceNotFoundError/ ChromaBackend from upstream). - mempalace/palace_graph.py — adopted upstream's null-safe wing filter (handles `"source": null` in hand-edited tunnels.json) while keeping the fork's variable naming for the downstream `explicit_tagged` step. - mempalace/searcher.py — combined upstream's filesystem-first State A/B pre-checks (MemPalace#1498) ahead of fork's `search_memories` delegation so read-only search of an empty palace dir no longer mutates the dir as a side effect. - tests/test_config.py — additive: kept both kg_backend tests (fork) and chunk-config validation tests (upstream). - tests/test_hooks_cli.py — dropped fork's `normalize_wing_name`-based tests in favor of upstream's hyphenated-project + cwd-from-jsonl tests, matching the new `_wing_from_transcript_path` API adopted in hooks_cli.py. - tests/test_searcher.py — kept fork's `degrades_to_warning` behavior (consistent with the `search_memories` delegation path that traps RuntimeError into a warning + sqlite fallback) and threaded `fake_palace_path` fixture through. Upstream features now live on the fork: - PR MemPalace#1024 configurable chunking + MemPalace#1519 (Igor) min_chunk_size validation - KG cache path canonicalization (realpath + normcase, symlink/case alias coalescing) - Cold-start embedder diagnostics + opt-in warmup (MemPalace#1495) - None-metadata MCP handler fix (_safe_meta) - Tunnel hyphenated wing-slug preservation + null-safe tunnel filter - Stratified palace state messages (State A/B/C/D, MemPalace#1498) - Bash 3.2 compat for hooks - Entity-registry tmp cleanup on failure Fork-only features preserved: - Postgres+pgvector+AGE substrate (kg_backend=age routing in _get_kg) - palace-daemon hook routing via clients/hook.py - Multi-encoder RRF research feature - Soft-warn on non-canonical room names - Hybrid_v4 retrieval logic - Postgres-direct aggregate path for build_graph - hook_verbatim_mode convo knob - symbol_header_prefix chunk enrichment hook (MemPalace#1384) TODO markers added: 1 (hooks_cli.py wing-derivation reconcile against palace-daemon's convention). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…k timeout bounds, update wing API expectations Three follow-ups to the 60-commit upstream/develop merge (6058489), each revealed by the full pytest run and addressed without re-opening the merge. mempalace/config.py: Dropped the fork's legacy raw-passthrough ``chunk_size`` / ``chunk_overlap`` / ``min_chunk_size`` @Property definitions (lines ~632-645). They shadowed upstream's validated, coercing versions defined earlier in the class (~L482) — Python resolves the later @Property assignment so the validation that MemPalace#1024 introduced never ran. With these removed, the validated accessors are the single source of truth and ingest no longer crashes on hand-edited config.json with a string/bool/negative value. tests/test_claude_plugin_hook_config.py: Upstream's new schema test (MemPalace#1465) bounded hook timeouts in *seconds* (10-30 for Stop, 60-90 for PreCompact) — matching their inline ``mempalace hook run`` subprocess invocation. The fork routes hooks through palace-daemon/clients/hook.py and Claude Code interprets the ``timeout`` field as *milliseconds* (consistent with JP's ~/.claude/settings.json which uses 5000/10000/130000). Scaled the bounds accordingly: SessionStart 1000-10000, Stop 10000-30000, PreCompact 10000-90000. Added the SessionStart entry too — the fork's plugin config declares it; upstream's didn't. tests/test_hooks_cli.py: Updated wing-name expectations to match the upstream MemPalace#1410 API adopted in the merge: _wing_from_transcript_path now returns ``wing_<project>`` rather than the fork's bare normalize_wing_name output. Affected assertions: - test_stop_hook_saves_silently_at_interval - test_stop_hook_derives_wing_from_transcript_path - test_wing_from_transcript_path_extracts_project - test_wing_from_transcript_path_fallback - test_wing_from_transcript_path_windows_backslashes - test_wing_from_transcript_path_lowercases - test_wing_from_transcript_path_macos_users_layout - test_ingest_transcript_routes_through_daemon (the wing= kwarg) - test_ingest_transcript_skips_when_target_running (the cmd's --wing arg) - test_precompact_mines_transcript_dir (the cmd's --wing arg) Full suite: 2105 passed, 33 skipped, 106 deselected. Pre-existing fork-side test-isolation failures in test_hooks_cli.py (visible when the file runs alone but masked in the full pytest run where earlier tests warm up the temp ``~/.mempalace`` so ``_palace_root_exists()`` returns True) are unchanged from the fork's pre-merge tip (aad0a14) and out of scope for this commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…merges (#112) Five PRs landed today that didn't have YAML entries yet; CLAUDE.md + README still referenced the pre-transfer jphein/mempalace ownership; the opencode integration recipe didn't cover the two gotchas that bit us tonight (npm-vs-opencode-cache patch target; the "looks like an error" stderr lines from local-palace spawns). YAML additions (newest first): - local-palace-retired-marker (#111 — 798cf14) - opencode-repo-config-empty-mcp (#110 — 7133eee) - opencode-repo-mcp-drop-comment-key (#109 — 637bb01) - opencode-repo-mcp-disable-flag (#108 — 47018e5; superseded by #110) - mcp-stub-resources-prompts-list (#107 — 6ca0670) CLAUDE.md changes: - Fork URL: `jphein/mempalace` → `techempower-org/mempalace` - Origin/upstream line reflects the May 2026 transfer - Palace data section: local palace is retired; production lives in postgres on disks.jphe.in - Python venv: `./venv/` → `./.venv/` (uv convention) - Issue tracker links updated to techempower-org README.md changes: - Quickstart and `Open upstream PRs` table use techempower-org URLs docs/integrations/opencode.md changes: - New "Patch target gotcha" subsection: opencode caches plugins under ~/.cache/opencode/packages/ independently of the global npm install; patches must apply there. - New "What 'looks like an error' but isn't" verification subsection listing the three stderr lines that surfaced today during debug. - New "If you see a different palace count" troubleshooting block: palace-count divergence between MCP and CLI usually means an old shell without PALACE_DAEMON_URL; on this fork the local palace is retired so the CLI now refuses with the marker text. FORK_CHANGELOG.md regenerated via scripts/render-docs.py. check-docs.sh clean (29 fork hash refs resolve; only existing MemPalace#1024 pr_state warning, unchanged). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: update jphein/ URLs to techempower-org/ (#118) - render-docs.py: changelog header and commit_link() now point at techempower-org/mempalace so the FORK_CHANGELOG regenerates with the correct repo slug. - check-docs.sh: cross-repo URL filter accepts both jphein/ and techempower-org/ slugs while the YAML body still carries historical jphein/ narrative; the retired-check comment updated for symmetry. - README.md: sweep all https://github.com/jphein/mempalace and https://github.com/jphein/palace-daemon URLs to their techempower-org equivalents (PR links, commit links, deployment-runs-at slug). - FORK_CHANGELOG.md: regenerated from docs/fork-changes.yaml so every rendered commit link resolves to techempower-org/mempalace; the historical body text inside YAML entries is unchanged. Historical narrative ("transferred from jphein/mempalace in May 2026") and links to other personal forks (jphein/familiar.realm.watch, jphein/notebook, etc.) stay as-is — only the two repos that actually moved to techempower-org are rewritten. Closes #118 * docs: mark PR MemPalace#1024 merged, MemPalace#1085 closed; drop stale ChromaDB references - docs/fork-changes.yaml: PR MemPalace#1024 (configurable chunking) moved from OPEN to MERGED with merged_date 2026-05-15; added to merged_upstream entries so the FORK_CHANGELOG recently-merged section reflects the 2026-05-15 upstream land. - docs/fork-changes.yaml: PR MemPalace#1085 (batched ChromaDB inserts cherry-pick) moved from OPEN to CLOSED with body note that @midweste closed it 2026-05-16, superseded by merged upstream MemPalace#1185 (wider GPU-acceleration scope). Fork's 6be6fff is now a no-op against develop. - CLAUDE.md: Key Files block rewrites the stale "ChromaDB vector store. The actual data." line to point at postgres on disks.jphe.in via palace-daemon and reference the 2026-05-14 cutover + RETIRED marker. - CLAUDE.md: Two-Layer Memory Architecture line updates MemPalace storage from "~/.mempalace/palace/, ~183K drawers" to the production postgres + pgvector + AGE on disks.jphe.in with a normalized "300K+" drawer count. README.md drawer-count normalization (160K/273K/274K -> 300K+) and ChromaDB->pgvector edits landed in efcb5c6 alongside the #118 URL sweep. FORK_CHANGELOG.md regenerates clean from the updated YAML. Closes #115, #116 * fix(docs): unbreak check-docs test-count lint and converge on .venv The README test-count drift check in scripts/check-docs.sh anchored its regex at start-of-line, but the count lives mid-paragraph on README:28. The check silently no-op'd, letting "~1850 tests pass on main" drift to 2486 actual without a single warning (#114). De-anchor the regex; the rest of the pipeline already does the right thing (counts via pytest --collect-only). Also converge on .venv/ — the project's actual venv path — instead of the dead venv/ path hard-coded in scripts/check-docs.sh and scripts/preflight.sh. Fixes the silent fallback-to-PATH behavior that masked the wrong path for so long (#117). CLAUDE.md:59 was already inconsistent with :46 — both now say .venv. Refs: #114, #117
848774c (fix/repair-empty-metadata branch) and df9187c (MemPalace#1024 PR branch) no longer resolve after squash-merge. Remove the backtick- quoted hashes from narrative text — the surviving cherry-pick (949cb20) and PR references remain. Fixes check-docs.sh step 2/4 on feat/check-docs-ci. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Why
Chunk sizing in the miner is currently hardcoded via three module-level constants in
mempalace/miner.py:One size does not fit all. Source material varies enormously — dense source code wants smaller, tighter chunks; prose transcripts and journal entries work better with larger, more overlapping chunks; sparse logs want very small
min_chunk_sizeto avoid dropping short but meaningful entries. Users also have different context-window budgets: a palace driving a 200K-token Claude session can afford larger drawers than one driving an 8K local model.Today the only way to change these values is to edit source and reinstall. This PR makes all three overridable from
~/.mempalace/config.jsonwithout changing defaults.What
Adds three new optional keys to
config.json:{ "chunk_size": 1200, "chunk_overlap": 150, "min_chunk_size": 40 }chunk_size(default800) — target character count per drawerchunk_overlap(default100) — characters shared between adjacent chunks so boundary information is never lostmin_chunk_size(default50) — files (and tail chunks) smaller than this are skippedExposed as
MempalaceConfigproperties:Threaded through the mining path as optional keyword arguments:
chunk_text()andprocess_file()keep the module-level constants as fallbacks whenever a parameter isNone, so any caller that omits the new kwargs gets identical behavior to before this PR.Backwards compatibility
800 / 100 / 50config.jsonkeys are optional — missing keys fall through to the same defaultschunk_text(content, source_file)— the original two-arg signature — still worksTests
No test changes required — defaults match existing hardcoded values exactly.
Scope note
The source commit on my fork (
d63ffd4) bundled three changes:Layer1.MAX_SCAN=2000cap on wake-up scan_build_where_filter()helper extracted insearcher.pyUpstream
developalready contains equivalents of I11 and I12, which made a straight cherry-pick produce tangled conflicts. I reconstructed the chunking slice cleanly on top ofupstream/develop— I11 and I12 are therefore not in this PR and not needed.Test plan
pytest -k "chunk or config"— 62 passedpytest -k "miner"— 48 passedMempalaceConfig().chunk_size / .chunk_overlap / .min_chunk_sizereturn800 / 100 / 50with noconfig.jsonpresent"chunk_size": 1200in~/.mempalace/config.jsonpropagates throughmine() → process_file() → chunk_text()