Skip to content

fix(mcp): forward valid_to and source params in kg_add/kg_invalidate (#1314)#1320

Merged
igorls merged 3 commits intodevelopfrom
fix/1314-kg-temporal-params
May 3, 2026
Merged

fix(mcp): forward valid_to and source params in kg_add/kg_invalidate (#1314)#1320
igorls merged 3 commits intodevelopfrom
fix/1314-kg-temporal-params

Conversation

@igorls
Copy link
Copy Markdown
Member

@igorls igorls commented May 3, 2026

Closes #1314.

Summary

Three knowledge-graph parameters were accepted by the MCP API but silently dropped before storage, making historical backfill lose precision:

  • mempalace_kg_add(valid_to=...) was never on the handler signature. The underlying KnowledgeGraph.add_triple already accepts it (alongside source_file / source_drawer_id from RFC 002 §5.5), so this was a pure boundary-wiring bug.
  • mempalace_kg_invalidate resolved a missing ended to date.today() inside SQLite but returned the literal string "today" to the caller, hiding the actual stamped date.

Changes

  • Extend tool_kg_add signature + MCP input_schema to accept valid_to, source_file, and source_drawer_id; forward all of them to _kg.add_triple and the WAL log.
  • Resolve ended to date.today().isoformat() in tool_kg_invalidate before logging / returning so the response always reports the date that was actually persisted.
  • Three new regression tests in tests/test_mcp_server.py:
    • test_kg_add_forwards_valid_to — round-trips valid_to through add_triple and asserts current=False.
    • test_kg_add_forwards_source_provenance — asserts source_closet / source_file / source_drawer_id all reach the SQLite row.
    • test_kg_invalidate_returns_actual_ended_date — asserts explicit and implicit ended both come back as ISO dates (never "today").
  • TODO comments referencing fix(kg): validate ISO-8601 date formats in temporal parameters at MCP boundary #1283 mark the spots where validate_iso_date should chain in once that PR lands. No regression risk to fix(kg): validate ISO-8601 date formats in temporal parameters at MCP boundary #1283: the new params land in the same kwarg style and don't bypass any validation hook.

Coordination with #1283

PR #1283 adds validate_iso_date() for as_of / valid_from / ended. This PR doesn't import it (still unmerged) but leaves TODO(#1283) markers next to each new date param so a follow-up is a one-line wire-up.

Test plan

  • python -m pytest tests/ -v --ignore=tests/benchmarks -x -k "kg or knowledge" — 59 passed, 1415 deselected.
  • ruff check . — all passed.
  • ruff format --check mempalace/mcp_server.py tests/test_mcp_server.py — already formatted.

Generated with Claude Code.

…1314)

`tool_kg_add` previously accepted only `valid_from` and `source_closet`,
silently dropping `valid_to`, `source_file`, and `source_drawer_id` at
the MCP boundary. Backfilling already-ended historical facts therefore
collapsed to "still current," and adapter provenance never reached
the SQLite layer even though `KnowledgeGraph.add_triple` already
supported every column.

`tool_kg_invalidate` returned the literal string `"today"` whenever the
caller omitted `ended`, hiding the actual stamped date from anyone trying
to verify what got persisted.

Changes:
- Extend `tool_kg_add` signature + MCP input_schema with `valid_to`,
  `source_file`, `source_drawer_id`; forward all of them to
  `_kg.add_triple` and to the WAL log.
- Resolve `ended` to `date.today().isoformat()` in `tool_kg_invalidate`
  before logging / returning, so the response always reports the actual
  date stored in `valid_to`.
- Add regression tests for valid_to round-trip, source_file /
  source_drawer_id provenance, and the resolved-ended-date contract.
- Leave TODO(#1283) markers so the open ISO-8601 validation PR can drop
  `validate_iso_date` over `valid_from` / `valid_to` / `ended` cleanly.

The underlying `KnowledgeGraph.add_triple` already accepted these
kwargs (RFC 002 §5.5) — only the MCP edge needed wiring up.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Copilot AI review requested due to automatic review settings May 3, 2026 01:54
@igorls igorls added bug Something isn't working area/mcp MCP server and tools labels May 3, 2026
@igorls igorls added this to the v3.3.5 milestone May 3, 2026
@igorls igorls added the area/kg Knowledge graph label May 3, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@igorls igorls merged commit 5380189 into develop May 3, 2026
6 checks passed
The repo's anti-jargon meta-test bans §N markers outside the
sources/backends allowlist. mcp_server.py isn't allowlisted, so the
"RFC 002 §5.5" references added in this PR turned the test red.
Trim to "RFC 002" — section number isn't load-bearing for the description.
xcarbo added a commit to xcarbo/mempalace that referenced this pull request May 4, 2026
Catches up on a month of upstream work. Highlights pulled in:

- MemPalace#1306 searcher: hybrid candidate union (vector ∪ BM25 reranking pool)
- MemPalace#1325 mcp security: omit absolute paths from tool_get_drawer / tool_status
- MemPalace#1322 chroma: wire quarantine_stale_hnsw to prevent SIGSEGV on stale HNSW
- MemPalace#1320 mcp: forward valid_to / source params in kg_add / kg_invalidate
- MemPalace#1321 cli: honor --palace flag in cmd_init
- MemPalace#1314 kg temporal params fix
- MemPalace#1244 cli: cmd_compress writes to mempalace_closets so palace can read
- MemPalace#1243 mcp: case-insensitive agent name in diary_write/diary_read
- MemPalace#1303 mcp_server: pass embedding_function= on collection reopen
- MemPalace#1076/MemPalace#1077 hooks: quote CLAUDE_PLUGIN_ROOT / CODEX_PLUGIN_ROOT in hooks.json
- Various ruff format passes on touched files

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

- 3.3.5 unreleased section header from upstream kept above 3.3.4
- 3.3.4 section: kept our 2026-04-30 release date; merged upstream's new
  MemPalace#1299 SIGSEGV-on-default-EF entry in alongside our existing topic-tunnels
  (MemPalace#1194/MemPalace#1195/MemPalace#1197), HNSW-bloat (MemPalace#1191), max_seq_id (MemPalace#1135), and
  auto-ingest (MemPalace#1230/MemPalace#1231) entries. Kept our richer topic-tunnels detail
  (upstream's version was a strict subset).

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

Not pushed to origin — run tests locally and decide when to push.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/kg Knowledge graph area/mcp MCP server and tools bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kg_add and kg_invalidate silently drop temporal/source params (valid_to, source) — backfill loses precision

2 participants