Skip to content

fix(kg): reject inverted intervals in add_triple (valid_to < valid_from) - #1214

Merged
igorls merged 1 commit into
MemPalace:developfrom
arnoldwender:fix/kg-temporal-inversion-guard
May 6, 2026
Merged

igorls merged 1 commit into
MemPalace:developfrom
arnoldwender:fix/kg-temporal-inversion-guard

Conversation

@arnoldwender

Copy link
Copy Markdown
Contributor

What and Why

A triple with valid_to before valid_from satisfies neither of the temporal filter clauses in KnowledgeGraph.query_entity():

valid_from <= as_of AND valid_to >= as_of

so the triple is invisible to every query — silently corrupt. The data lives in SQLite forever but never surfaces, and the caller never sees a warning. This is a P0 data-integrity bug in a path adapters can hit easily (any caller that mixes up the two date params).

Root Cause

mempalace/knowledge_graph.py:149add_triple() accepts valid_from and valid_to as opaque strings, validates each format independently in upstream callers (and now PR #1167 at the MCP boundary), but never checks the relationship between them.

Fix

Reject at write time with a clear ValueError naming both bounds. The guard fires only when both are set:

  • Open intervals (only valid_from, or only valid_to) — accepted unchanged.
  • Same-day intervals (valid_from == valid_to, point-in-time facts) — explicitly allowed (< not <=).
  • String comparison is correct because sanitize_iso_date (PR fix(kg): validate ISO-8601 date formats at MCP boundary #1167) ensures YYYY-MM-DD lex order matches calendar order.

Test plan

  • test_add_triple_rejects_inverted_interval — asserts ValueError with 'before valid_from' match
  • test_add_triple_accepts_equal_dates — point-in-time facts pass
  • test_add_triple_allows_only_one_bound — open intervals (one bound) still pass
  • All 24 test_knowledge_graph.py tests green
  • Full suite: 1318 passed locally (2 pre-existing failures in test_backends.py::test_pin_hnsw_threads* unrelated — environmental ChromaDB configuration_json["hnsw"] schema drift, also fails on a clean develop checkout)

A triple with valid_to < valid_from satisfies neither of the temporal
filter clauses in query_entity():

    valid_from <= as_of AND valid_to >= as_of

so the triple is invisible to every query — silently corrupt. Reject
at write time with a clear error instead of letting bad data pile up
in the SQLite store.

The guard only fires when both bounds are present; open intervals
(only valid_from or only valid_to) are still accepted, and same-day
intervals (valid_from == valid_to, point-in-time facts) are explicitly
allowed.
@jphein

jphein commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

+1 to rejecting at write time — silently invisible triples are a hard failure mode to debug after the fact.

One coordination note worth surfacing: lex-string valid_to < valid_from is correct only once both sides are canonical YYYY-MM-DD. Counterexample without sanitize_iso_date on the boundary:

"2026-12-01" < "2026-3-01"  # True — the guard would falsely reject Dec→Mar as inverted

The PR description names the dependency on #1167. Just noting that if #1167 stalls and #1214 lands first, MCP tool_kg_add callers passing non-zero-padded months would hit spurious ValueError. Tests in this PR all use canonical form so they don't exercise the gap.

Low-risk in practice (LLM-driven callers tend to emit ISO 8601), and the right fix shape is #1167 anyway. Not blocking — just worth tagging the dependency in case the merge order goes #1214#1167 rather than the other way.

@arnoldwender

Copy link
Copy Markdown
Contributor Author

@jphein Thanks — good call on tagging the merge-order gap explicitly. Concrete behavior if #1214 lands before #1167:

  • MCP tool_kg_add callers passing non-zero-padded months like "2026-3-01" would hit ValueError: valid_to < valid_from even on valid intervals, because lex-string "2026-12-01" < "2026-3-01" is True.
  • LLM-driven callers tend to emit canonical ISO 8601, so practical exposure is low.
  • The right fix is fix(kg): validate ISO-8601 date formats at MCP boundary #1167 (sanitize_iso_date at the MCP boundary), not duplicate validation here.

Happy with either merge order. If #1167 first, the gap closes naturally; if #1214 first, the limitation is documented and #1167 follows up.

@jphein

jphein commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Thanks @arnoldwender — concrete on the merge-order behavior. Your read matches mine: practical exposure is small because LLM-driven callers tend to emit canonical ISO 8601, but the gap is worth the explicit docstring note in the meantime. Happy to defer to whichever of #1167 / #1214 lands first; the other will get a small rebase note if needed.

@igorls igorls added the bug Something isn't working label May 2, 2026
@igorls igorls added this to the v3.3.5 milestone May 2, 2026
@igorls
igorls merged commit 0cfb4b3 into MemPalace:develop May 6, 2026
6 checks passed
igorls added a commit that referenced this pull request May 6, 2026
…1282 #1167 #1160

Bundled CHANGELOG entries for the seven Tier-1 PRs merged today, including
the behavior-change call-out for #1167 (KG date validators now reject
non-ISO inputs that previously produced silent empty results).
xcarbo added a commit to xcarbo/mempalace that referenced this pull request May 7, 2026
Catches up on a heavy upstream day — 22 fixes merged in 24h plus prior
backlog. Highlights pulled in:

- MemPalace#1305 hooks: ~/.mempalace/ deletion is now a stable kill-switch (hooks
  no longer rebuild the dir hierarchy on Stop/PreCompact/SessionStart)
- MemPalace#1214 KG: reject inverted intervals (valid_to < valid_from) at write time —
  prevents silently invisible triples
- MemPalace#1067/MemPalace#1105 chroma: ChromaBackend.close_palace() now actually releases
  the SQLite file lock (PersistentClient.close() on evict + invalidation)
- MemPalace#1215 entity_registry: atomic save (tmp+fsync+rename) — no more
  corruption on crash mid-write
- MemPalace#1073/MemPalace#1107 mempalace compress: paginated drawer fetch — no longer
  trips SQLITE_MAX_VARIABLE_NUMBER on palaces >32k drawers
- MemPalace#1282 stdio: Windows console UTF-8 reconfig for cli/mcp_server/hooks_cli
- MemPalace#1164/MemPalace#1167 mcp KG: sanitize_iso_date() blocks malformed date strings
  silently producing empty result sets
- MemPalace#1136/MemPalace#1160 mcp: per-path KG cache for multi-tenant hosts that rotate
  MEMPALACE_PALACE_PATH between tool calls
- MemPalace#1286 mcp: retry _get_collection() once on transient failure
- MemPalace#1138 lint cleanup, MemPalace#1019 search-crash fix
- 4 new tools/ scripts (backup_claude_jsonls, find_orphan_claude_jsonls,
  render_jsonl, save.md)

Conflict resolution (CHANGELOG.md only — code files all auto-merged):

- 3.3.5 section: untouched (already merged in our prior commit; upstream
  added several new bug-fix entries which auto-merged cleanly)
- 3.3.4 Bug Fixes: kept upstream's new MemPalace#1305 entry; preserved our richer
  detail on topic-tunnels (MemPalace#1194/MemPalace#1195/MemPalace#1197), HNSW-bloat (MemPalace#1191),
  max_seq_id (MemPalace#1135), and auto-ingest (MemPalace#1230/MemPalace#1231) — upstream's shorter
  topic-tunnels entry was a strict subset of ours.

xdev patches preserved (still on this branch, untouched by merge):
- 6ef44cb fix(hooks): route CC transcripts via convo_miner with cwd-based wings
- 3fad61d fix(config): allow leading dash in wing names
- 3fc821a fix(config): tighten leading-char to allow dash but not underscore

Tests: 1557 passed, 1 skipped (full unit suite excluding benchmarks).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@igorls igorls mentioned this pull request May 10, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants