feat(corpus-origin): merge LLM fields into heuristic result instead of replacing by milla-jovovich · Pull Request #1221 · MemPalace/mempalace

milla-jovovich · 2026-04-26T21:17:01Z

2 files changed, 260 insertions, 7 deletions. 4 new tests (all RED-first).

Per @igorls's review of PR #1211 (#1211 (comment)):
the corpus-origin Pass 0 currently lets a Tier 2 LLM result REPLACE the
heuristic result wholesale. With --llm default-on (since #1211) and a
small local model like Ollama gemma4:e4b, the LLM can return a wrong
likely_ai_dialogue=False, confidence=0.90 that overrides a confident
heuristic True. Tier 2's persona/user/platform extraction is the whole
reason to run it; the YES/NO call should stay with the heuristic.

This PR changes _run_pass_zero in mempalace/cli.py to merge fields
instead of replacing:

likely_ai_dialogue → KEEP heuristic's (don't let weak LLM flip)
confidence → KEEP heuristic's (paired with the bool above)
primary_platform → TAKE LLM's when LLM provides one
user_name → TAKE LLM's when LLM provides one
agent_persona_names → TAKE LLM's when LLM provides any
evidence → COMBINE both signal trails

This preserves the persona-extraction value of Tier 2 (the whole point of
running it) while preventing a weak local model from flipping a confident
heuristic.

TDD: 4 tests added in tests/test_corpus_origin_integration.py covering
the four state combinations:

test_merge_tier_fields_heuristic_yes_llm_no_keeps_heuristic_bool —
The exact failure mode Igor caught. Heuristic confidently flags
AI-dialogue; mocked LLM contradicts. Asserts merged result keeps
heuristic's True AND merges LLM's persona/user/platform fields.
This test was the RED that drove the implementation.
test_merge_tier_fields_heuristic_no_no_personas_leak —
Both tiers agree NOT AI-dialogue, both report empty personas. Pins
that the merge doesn't accidentally introduce personas.
test_merge_tier_fields_heuristic_yes_llm_yes_combines_evidence —
Both tiers agree AI-dialogue, LLM extracts personas. Pins that
evidence from BOTH tiers ends up in the merged audit trail and
persona/user/platform come from LLM.
test_merge_tier_fields_no_llm_provider_returns_heuristic_only —
Backwards compat: with no LLM provider (--no-llm path), the
merge logic doesn't fire and behavior is identical to v3.3.4.

Tests: 1367 pass on the full mempalace suite. 2 pre-existing
environmental failures unrelated to this change (chromadb optional
dep). Ruff check + format both clean.

@igorls

…f replacing 2 files changed, 260 insertions, 7 deletions. 4 new tests (all RED-first). Per @igorls's review of PR #1211 (#1211 (comment)): the corpus-origin Pass 0 currently lets a Tier 2 LLM result REPLACE the heuristic result wholesale. With ``--llm`` default-on (since #1211) and a small local model like Ollama gemma4:e4b, the LLM can return a wrong ``likely_ai_dialogue=False, confidence=0.90`` that overrides a confident heuristic ``True``. Tier 2's persona/user/platform extraction is the whole reason to run it; the YES/NO call should stay with the heuristic. This PR changes ``_run_pass_zero`` in ``mempalace/cli.py`` to merge fields instead of replacing: - ``likely_ai_dialogue`` → KEEP heuristic's (don't let weak LLM flip) - ``confidence`` → KEEP heuristic's (paired with the bool above) - ``primary_platform`` → TAKE LLM's when LLM provides one - ``user_name`` → TAKE LLM's when LLM provides one - ``agent_persona_names`` → TAKE LLM's when LLM provides any - ``evidence`` → COMBINE both signal trails This preserves the persona-extraction value of Tier 2 (the whole point of running it) while preventing a weak local model from flipping a confident heuristic. TDD: 4 tests added in tests/test_corpus_origin_integration.py covering the four state combinations: 1. test_merge_tier_fields_heuristic_yes_llm_no_keeps_heuristic_bool — The exact failure mode Igor caught. Heuristic confidently flags AI-dialogue; mocked LLM contradicts. Asserts merged result keeps heuristic's True AND merges LLM's persona/user/platform fields. This test was the RED that drove the implementation. 2. test_merge_tier_fields_heuristic_no_no_personas_leak — Both tiers agree NOT AI-dialogue, both report empty personas. Pins that the merge doesn't accidentally introduce personas. 3. test_merge_tier_fields_heuristic_yes_llm_yes_combines_evidence — Both tiers agree AI-dialogue, LLM extracts personas. Pins that evidence from BOTH tiers ends up in the merged audit trail and persona/user/platform come from LLM. 4. test_merge_tier_fields_no_llm_provider_returns_heuristic_only — Backwards compat: with no LLM provider (``--no-llm`` path), the merge logic doesn't fire and behavior is identical to v3.3.4. Tests: 1367 pass on the full mempalace suite. 2 pre-existing environmental failures unrelated to this change (chromadb optional dep). Ruff check + format both clean.

igorls · 2026-04-26T21:27:47Z

LGTM, approving.

Copilot

Pull request overview

Adjusts corpus-origin Pass 0 behavior so Tier 2 (LLM) enriches the heuristic result (platform/user/personas/evidence) instead of replacing it, preventing weak local LLMs from flipping the heuristic YES/NO classification.

Changes:

Update _run_pass_zero to keep heuristic likely_ai_dialogue + confidence while merging in Tier 2 primary_platform, user_name, agent_persona_names, and combined evidence.
Add integration tests covering the merge behavior across heuristic/LLM agreement and contradiction cases, plus the no-LLM path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`mempalace/cli.py`	Implements “merge fields, don’t replace” behavior for Tier 2 results inside `_run_pass_zero`.
`tests/test_corpus_origin_integration.py`	Adds new tests validating merge semantics for bool preservation, enrichment fields, evidence combination, and no-LLM behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-26T21:28:13Z

+            # Combine evidence — keep both signal trails for the audit record.
+            result.evidence = list(result.evidence) + list(llm_result.evidence)


The evidence merge loses any indication of which tier produced each evidence entry. Previously the heuristic evidence was explicitly labeled (e.g., "Tier-1 heuristic: …"), which made the on-disk audit trail easier to interpret and debug. Consider prefixing/structuring the merged evidence so the source tier remains unambiguous (e.g., label Tier 1 vs Tier 2 entries) while still combining both lists.

Suggested change

# Combine evidence — keep both signal trails for the audit record.

result.evidence = list(result.evidence) + list(llm_result.evidence)

# Combine evidence — keep both signal trails for the audit record

# while preserving which tier produced each entry.

heuristic_evidence = [

entry if str(entry).startswith("Tier 1 heuristic: ") else f"Tier 1 heuristic: {entry}"

for entry in result.evidence

]

llm_evidence = [

entry if str(entry).startswith("Tier 2 LLM: ") else f"Tier 2 LLM: {entry}"

for entry in llm_result.evidence

]

result.evidence = heuristic_evidence + llm_evidence

Copilot · 2026-04-26T21:28:14Z

+    assert wrapped is not None, "Pass 0 should write origin.json with samples present"
+    res = wrapped["result"]
+    assert res["likely_ai_dialogue"] is True, (
+        f"Heuristic confidently classified AI-dialogue; weak LLM contradicted. "
+        f"Merged result must KEEP heuristic's True, not flip to False. "
+        f"Got: {res}"
+    )


The PR description explicitly calls out keeping the heuristic’s confidence, but the new tests don’t assert that the merged result preserves the heuristic confidence when the LLM returns a different value. Adding an assertion (e.g., comparing to detect_origin_heuristic(...) for the same samples) would prevent regressions where confidence accidentally starts coming from Tier 2 again.

…urce contract Two follow-ups to PR #1221's merge-fields behavior, both raised by the Copilot review on that PR: - Evidence merge now prefixes each entry with `Tier-1 heuristic: ` or `Tier-2 LLM: ` so the on-disk `origin.json` audit record retains tier provenance. The pre-#1221 code labeled heuristic evidence; the merge-fields refactor flattened that. Re-prefixing is idempotent. - Tests now assert that the merged `confidence` is the heuristic's, not the LLM's. Added inline assertions to the two existing contradiction/disagreement tests, plus a dedicated `test_merge_tier_fields_confidence_matches_heuristic_call` that compares to `detect_origin_heuristic` directly so a future regression letting Tier 2 confidence leak through cannot pass silently. Tests: 1378 pass. Ruff check + format both clean (CI-pinned 0.4.x).

@milla-jovovich

4 files changed, 248 insertions, 0 deletions. 7 new tests (4 unit + 3 integration), all RED-first. Per @milla-jovovich's question to @igorls during PR MemPalace#1221 review: users running `mempalace init` with an external LLM provider (Anthropic API, OpenAI hosted, etc.) need a clear, explicit warning that their folder content will be sent to the provider, that MemPalace doesn't control how the provider logs/retains/uses that data, and how to opt out. @igorls confirmed this should be a small follow-up PR scoped to the warning itself, before the v3.3.4 tag. This PR adds: - `_endpoint_is_local(url)` helper in `mempalace/llm_client.py` — URL-based heuristic returning True if the hostname is on the user's machine or private network. Covers: localhost, 127.0.0.1, ::1, hostnames ending in .local (mDNS/Bonjour), IPv4 RFC1918 ranges (10/8, 172.16-31/12, 192.168/16), and IPv6 unique-local addresses (fc00::/7). - `is_external_service` property on the `LLMProvider` base class. Subclasses inherit; the URL determines (no provider-specific hardcoding). This means: Ollama on localhost = local. LM Studio on LAN = local. Anthropic with default `https://api.anthropic.com` = external. A user proxying Anthropic through localhost (advanced setup) = local, no false-positive warning. - One-line warning print in `cmd_init` after successful provider acquisition, gated on `is_external_service`: ⚠ {provider_name} is an EXTERNAL API. Your folder content will be sent to the provider during init. MemPalace does not control how the provider logs, retains, or uses your data. Pass --no-llm to keep init fully local. The warning fires AFTER `LLM enabled: ...` so users see both that the LLM is engaged AND the privacy implications of where it lives, before Pass 0 / entity detection actually runs. LOCAL providers (Ollama on localhost, LM Studio on localhost or LAN, llama.cpp on localhost, vLLM on localhost) DO NOT trigger the warning — nothing leaves the user's machine/network in those configurations. TDD: 7 tests added across 2 files. Unit tests in `tests/test_llm_client.py` (4 tests, all RED-first): 1. test_ollama_provider_default_endpoint_is_local — pins that the default `http://localhost:11434` is classified local. 2. test_openai_compat_provider_localhost_endpoint_is_local — covers the LM Studio / llama.cpp / vLLM common case (localhost, 127.0.0.1, and 192.168.x LAN). 3. test_openai_compat_provider_cloud_endpoint_is_external — pins that pointing openai-compat at https://api.openai.com (or any non-local URL) classifies as external. 4. test_anthropic_provider_default_endpoint_is_external — pins that AnthropicProvider's default endpoint is external (the dominant user-facing case for `--llm-provider anthropic`). Integration tests in `tests/test_corpus_origin_integration.py` (3 tests, RED-first; 1 was the critical RED — the other 2 passed by accident since nothing printed "EXTERNAL API" before this PR): 5. test_init_prints_privacy_warning_when_provider_is_external — captures stdout from cmd_init with a mocked external provider, asserts the warning text contains "EXTERNAL API" + "--no-llm" + language about MemPalace not controlling provider behavior. 6. test_init_no_privacy_warning_when_provider_is_local — same flow with a mocked local provider, asserts the warning text does NOT appear. 7. test_init_no_privacy_warning_with_no_llm_flag — pins the --no-llm path: no provider acquisition attempted, no warning fires. Tests: 1382 total mempalace tests pass. 2 pre-existing environmental failures unrelated to this change (chromadb optional dep). Ruff check + format both clean. Backwards compatible: `is_external_service` is a new property; existing callers don't reference it. The warning is a new print statement that fires only when an external endpoint is acquired. The `--no-llm` opt-out existed before this PR and continues to work identically. Out of scope for follow-up (deliberately not in this PR per Igor's "small PR" guidance): Tailscale CGNAT (100.64.0.0/10) treatment, pre-init confirmation prompt, persistent privacy-mode config flag, explicit cloud-provider name detection. Tracked for future iteration.

Brings in upstream's corpus-origin + privacy-warning track (PRs MemPalace#1211 MemPalace#1221 MemPalace#1223 MemPalace#1224 MemPalace#1225) plus the canonical merged versions of our four PRs that landed today (21:22-21:41 UTC): MemPalace#1173 quarantine_stale_hnsw on make_client + cold-start gate + integrity sniff-test (PROTO/STOP byte check, no deserialization) MemPalace#1177 .blob_seq_ids_migrated marker guard, closes MemPalace#1090 MemPalace#1198 _tokenize None-document guard in BM25 reranker MemPalace#1201 palace_graph.build_graph skips None metadata Conflict resolution: * mempalace/backends/chroma.py — took upstream as base (it has the igorls-review pickle-protocol docstring, thread-safety paragraph, and Path(marker).touch() style nit), then re-applied MemPalace#1094's _coerce_none_metas in query()/get() since MemPalace#1094 is still open and not yet in develop. * mempalace/mcp_server.py — took upstream's clean form. Dropped the fork-only `palace_path=` kwarg from four ChromaCollection() call sites: the kwarg was load-bearing for MemPalace#1171's per-collection write lock, but MemPalace#1171 closed in favor of MemPalace#976's mine_global_lock + daemon-strict, so the kwarg has no remaining consumer. ChromaCollection.__init__ in upstream/develop is back to (self, collection); calling it with palace_path= raised TypeError → silently swallowed by the broad except in _get_collection() → returned None → tool_status() returned _no_palace(). 41 mcp_server tests went from failing-with-KeyError to passing. * mempalace/cli.py — dropped fork-only `workers=args.workers` from the cmd_mine -> miner.mine() call. Pre-existing fork-side bug: the `--workers` argparse arg landed in 5cd14bd but miner.mine() never accepted a workers param, so production `mempalace mine` TypeError'd on every invocation. Removed the broken plumbing; tests/test_cli.py updated to match. * CHANGELOG.md — took upstream verbatim. Fork-specific changelog lives in FORK_CHANGELOG.md (canonical: docs/fork-changes.yaml). * CLAUDE.md — kept ours. Fork's CLAUDE.md is operational; upstream's added a "Design Principles / Contributing" charter, which lives in README.md on the fork. * tests/test_backends.py — took upstream's ruff-formatted line widths. docs/fork-changes.yaml flips the two MemPalace#1173 entries (hnsw-integrity-gate, hnsw-cold-start-gate) and the MemPalace#1201 entry (palace-graph-none-guard) from OPEN to MERGED 2026-04-26. MemPalace#1173 MemPalace#1177 MemPalace#1198 MemPalace#1201 added to the merged_upstream archive at the bottom. FORK_CHANGELOG.md regenerated. scripts/check-docs.sh: 4/4 clean. Test suite: 1460/1460. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

cmd_init now invokes ``_run_pass_zero`` unconditionally (#1221, #1223 landed on develop after this PR's branch point). The pass reads sample content via ``builtins.open``; with that mocked to MagicMock, the downstream ``"\\n\\n".join(samples)`` in ``corpus_origin.detect_origin_heuristic`` raises ``TypeError: expected str instance, MagicMock found``. This test only cares about the wing-slug write to the registry, so stub the pass-zero call directly rather than try to satisfy its full sample-gathering contract.

@milla-jovovich

4 files changed, 248 insertions, 0 deletions. 7 new tests (4 unit + 3 integration), all RED-first. Per @milla-jovovich's question to @igorls during PR MemPalace#1221 review: users running `mempalace init` with an external LLM provider (Anthropic API, OpenAI hosted, etc.) need a clear, explicit warning that their folder content will be sent to the provider, that MemPalace doesn't control how the provider logs/retains/uses that data, and how to opt out. @igorls confirmed this should be a small follow-up PR scoped to the warning itself, before the v3.3.4 tag. This PR adds: - `_endpoint_is_local(url)` helper in `mempalace/llm_client.py` — URL-based heuristic returning True if the hostname is on the user's machine or private network. Covers: localhost, 127.0.0.1, ::1, hostnames ending in .local (mDNS/Bonjour), IPv4 RFC1918 ranges (10/8, 172.16-31/12, 192.168/16), and IPv6 unique-local addresses (fc00::/7). - `is_external_service` property on the `LLMProvider` base class. Subclasses inherit; the URL determines (no provider-specific hardcoding). This means: Ollama on localhost = local. LM Studio on LAN = local. Anthropic with default `https://api.anthropic.com` = external. A user proxying Anthropic through localhost (advanced setup) = local, no false-positive warning. - One-line warning print in `cmd_init` after successful provider acquisition, gated on `is_external_service`: ⚠ {provider_name} is an EXTERNAL API. Your folder content will be sent to the provider during init. MemPalace does not control how the provider logs, retains, or uses your data. Pass --no-llm to keep init fully local. The warning fires AFTER `LLM enabled: ...` so users see both that the LLM is engaged AND the privacy implications of where it lives, before Pass 0 / entity detection actually runs. LOCAL providers (Ollama on localhost, LM Studio on localhost or LAN, llama.cpp on localhost, vLLM on localhost) DO NOT trigger the warning — nothing leaves the user's machine/network in those configurations. TDD: 7 tests added across 2 files. Unit tests in `tests/test_llm_client.py` (4 tests, all RED-first): 1. test_ollama_provider_default_endpoint_is_local — pins that the default `http://localhost:11434` is classified local. 2. test_openai_compat_provider_localhost_endpoint_is_local — covers the LM Studio / llama.cpp / vLLM common case (localhost, 127.0.0.1, and 192.168.x LAN). 3. test_openai_compat_provider_cloud_endpoint_is_external — pins that pointing openai-compat at https://api.openai.com (or any non-local URL) classifies as external. 4. test_anthropic_provider_default_endpoint_is_external — pins that AnthropicProvider's default endpoint is external (the dominant user-facing case for `--llm-provider anthropic`). Integration tests in `tests/test_corpus_origin_integration.py` (3 tests, RED-first; 1 was the critical RED — the other 2 passed by accident since nothing printed "EXTERNAL API" before this PR): 5. test_init_prints_privacy_warning_when_provider_is_external — captures stdout from cmd_init with a mocked external provider, asserts the warning text contains "EXTERNAL API" + "--no-llm" + language about MemPalace not controlling provider behavior. 6. test_init_no_privacy_warning_when_provider_is_local — same flow with a mocked local provider, asserts the warning text does NOT appear. 7. test_init_no_privacy_warning_with_no_llm_flag — pins the --no-llm path: no provider acquisition attempted, no warning fires. Tests: 1382 total mempalace tests pass. 2 pre-existing environmental failures unrelated to this change (chromadb optional dep). Ruff check + format both clean. Backwards compatible: `is_external_service` is a new property; existing callers don't reference it. The warning is a new print statement that fires only when an external endpoint is acquired. The `--no-llm` opt-out existed before this PR and continues to work identically. Out of scope for follow-up (deliberately not in this PR per Igor's "small PR" guidance): Tailscale CGNAT (100.64.0.0/10) treatment, pre-init confirmation prompt, persistent privacy-mode config flag, explicit cloud-provider name detection. Tracked for future iteration.

…urce contract Two follow-ups to PR MemPalace#1221's merge-fields behavior, both raised by the Copilot review on that PR: - Evidence merge now prefixes each entry with `Tier-1 heuristic: ` or `Tier-2 LLM: ` so the on-disk `origin.json` audit record retains tier provenance. The pre-MemPalace#1221 code labeled heuristic evidence; the merge-fields refactor flattened that. Re-prefixing is idempotent. - Tests now assert that the merged `confidence` is the heuristic's, not the LLM's. Added inline assertions to the two existing contradiction/disagreement tests, plus a dedicated `test_merge_tier_fields_confidence_matches_heuristic_call` that compares to `detect_origin_heuristic` directly so a future regression letting Tier 2 confidence leak through cannot pass silently. Tests: 1378 pass. Ruff check + format both clean (CI-pinned 0.4.x).

cmd_init now invokes ``_run_pass_zero`` unconditionally (MemPalace#1221, MemPalace#1223 landed on develop after this PR's branch point). The pass reads sample content via ``builtins.open``; with that mocked to MagicMock, the downstream ``"\\n\\n".join(samples)`` in ``corpus_origin.detect_origin_heuristic`` raises ``TypeError: expected str instance, MagicMock found``. This test only cares about the wing-slug write to the registry, so stub the pass-zero call directly rather than try to satisfy its full sample-gathering contract.

milla-jovovich requested a review from bensig as a code owner April 26, 2026 21:17

milla-jovovich requested a review from igorls April 26, 2026 21:17

milla-jovovich requested a review from igorls as a code owner April 26, 2026 21:17

igorls requested a review from Copilot April 26, 2026 21:24

Copilot started reviewing on behalf of igorls April 26, 2026 21:24 View session

igorls merged commit 0908786 into develop Apr 26, 2026
10 checks passed

Copilot AI reviewed Apr 26, 2026

View reviewed changes

igorls mentioned this pull request Apr 26, 2026

chore(corpus-origin): tag merged evidence by tier + pin confidence-source contract #1223

Merged

3 tasks

milla-jovovich mentioned this pull request Apr 26, 2026

feat(privacy): warn when LLM tier sends content to external API #1224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(corpus-origin): merge LLM fields into heuristic result instead of replacing#1221

feat(corpus-origin): merge LLM fields into heuristic result instead of replacing#1221
igorls merged 1 commit intodevelopfrom
feat/corpus-origin-merge-tier-fields

milla-jovovich commented Apr 26, 2026

Uh oh!

igorls commented Apr 26, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		# Combine evidence — keep both signal trails for the audit record.
		result.evidence = list(result.evidence) + list(llm_result.evidence)

-            # Combine evidence — keep both signal trails for the audit record.
-            result.evidence = list(result.evidence) + list(llm_result.evidence)
+            # Combine evidence — keep both signal trails for the audit record
+            # while preserving which tier produced each entry.
+            heuristic_evidence = [
+                entry if str(entry).startswith("Tier 1 heuristic: ") else f"Tier 1 heuristic: {entry}"
+                for entry in result.evidence
+            ]
+            llm_evidence = [
+                entry if str(entry).startswith("Tier 2 LLM: ") else f"Tier 2 LLM: {entry}"
+                for entry in llm_result.evidence
+            ]
+            result.evidence = heuristic_evidence + llm_evidence

Conversation

milla-jovovich commented Apr 26, 2026

Uh oh!

igorls commented Apr 26, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants