Skip to content

fix(knowledge_graph): backfill NULL metadata on duplicate add_triple - #1139

Open
arnoldwender wants to merge 2 commits into
MemPalace:developfrom
arnoldwender:fix/kg-add-triple-backfill-null-metadata
Open

fix(knowledge_graph): backfill NULL metadata on duplicate add_triple#1139
arnoldwender wants to merge 2 commits into
MemPalace:developfrom
arnoldwender:fix/kg-add-triple-backfill-null-metadata

Conversation

@arnoldwender

Copy link
Copy Markdown
Contributor

What and Why

When add_triple() is called a second time with the same (subject, predicate, object) and the existing triple is still open (valid_to IS NULL), every new metadata value — valid_from, source_closet, source_file, source_drawer_id, adapter_name, confidence — was silently dropped:

if existing:
    return existing["id"]  # Already exists and still valid

This breaks the Incremental-only principle from CLAUDE.md: a later pass with better information should be additive, never a silent no-op. It is the exact "a later adapter refines what an earlier one did not know" pattern the RFC 002 §5.5 provenance fields (source_drawer_id, adapter_name) were designed to support.

CONTRIBUTING.md explicitly calls out knowledge_graph.py as an area where coverage is wanted.

Root Cause

mempalace/knowledge_graph.py:189-196

Change Summary

Policy for re-adding an existing open triple — preserves idempotency, adds provenance plumbing:

  • Backfill NULL fields when a new non-NULL value is provided
  • Never overwrite an already-populated field — invalidate() remains the only path to change settled values
  • Confidence is the one exception: a strictly higher value replaces a weaker one so stronger evidence wins; weaker evidence is ignored (confidence is monotonically non-decreasing)
  • The returned id is unchanged — idempotent contract preserved

Reproduction

from mempalace.knowledge_graph import KnowledgeGraph
kg = KnowledgeGraph(db_path=":memory:")

# Adapter 1 discovers the relationship, knows no start date:
tid = kg.add_triple("Max", "child_of", "Alice")

# Adapter 2 later finds the start date:
kg.add_triple("Max", "child_of", "Alice", valid_from="2015-04-01")

facts = kg.query_entity("Max", direction="outgoing")
print([f["valid_from"] for f in facts if f["predicate"] == "child_of"])

Before this PR: [None]
After this PR: ['2015-04-01']

Test Plan

Four regression tests covering every branch of the new policy:

  • test_duplicate_add_backfills_null_valid_from — NULL date gets filled
  • test_duplicate_add_backfills_null_provenance — RFC 002 provenance fields roundtrip
  • test_duplicate_add_does_not_overwrite_existing_metadata — locks the "never clobber" rule
  • test_duplicate_add_upgrades_confidence_only_when_higher — stronger evidence wins, weaker is ignored
  • Existing test_duplicate_triple_returns_existing_id and test_invalidated_triple_allows_re_add still pass — idempotent contract intact
  • pytest tests/ --ignore=tests/benchmarks — 1070/1070 pass, no downstream regressions
  • ruff check / ruff format --check — clean

Out of scope

Audit surfaced two adjacent correctness concerns I left for follow-up PRs to keep this one surgical:

  1. No validation that valid_from <= valid_to on input
  2. Lex comparison on mixed-precision timestamps in query_*(as_of=...) (e.g. '2025-10-01' vs '2025')

Happy to file them separately if this direction is welcome.

@igorls igorls added bug Something isn't working area/kg Knowledge graph labels Apr 24, 2026
@arnoldwender
arnoldwender force-pushed the fix/kg-add-triple-backfill-null-metadata branch 2 times, most recently from 5ee15fb to b04ec5b Compare May 1, 2026 12:41
@arnoldwender
arnoldwender force-pushed the fix/kg-add-triple-backfill-null-metadata branch 2 times, most recently from 5508e7b to 441c936 Compare May 10, 2026 11:02
@arnoldwender

Copy link
Copy Markdown
Contributor Author

Rebased on upstream/develop (latest 1247e17, post-3.3.5 release).

Conflict resolution: the test class TestTripleOperations in tests/test_knowledge_graph.py accumulated three new tests on develop (test_add_triple_rejects_inverted_interval, test_add_triple_accepts_equal_dates, test_add_triple_allows_only_one_bound — inversion-guard coverage from #1371). Those concerns are orthogonal to this PR (NULL-metadata backfill on duplicate add_triple), so I kept both test sets — develop's first, this PR's after. Production code in mempalace/knowledge_graph.py auto-merged cleanly.

Ran uv run pytest tests/test_knowledge_graph.py post-rebase: 40 passed.

@arnoldwender

Copy link
Copy Markdown
Contributor Author

Friendly ping — post-3.3.6 release this is still mergeable on upstream/develop @ f5ea021 (CI green, no conflicts). Happy to adjust anything if it would help land it.

@arnoldwender
arnoldwender force-pushed the fix/kg-add-triple-backfill-null-metadata branch from 441c936 to 6eb3b3c Compare May 30, 2026 12:49
@arnoldwender

Copy link
Copy Markdown
Contributor Author

Friendly ping — post-v3.5.0 status: knowledge_graph.py was untouched by the release, so this NULL-metadata backfill fix still applies cleanly (merges clean, CI green). Small, self-contained correctness fix; glad to add anything a reviewer would like.

@arnoldwender
arnoldwender force-pushed the fix/kg-add-triple-backfill-null-metadata branch from 6eb3b3c to c24a5d5 Compare July 22, 2026 19:44
@arnoldwender

Copy link
Copy Markdown
Contributor Author

Rebased onto current develop (aa89bd8, post-v3.6.0) — clean, no conflicts, no logical changes.

Re-verified that the bug is still live on develop rather than just re-pinging: knowledge_graph.py:311 still does the bare early-return on a duplicate triple —

if existing:
    return existing["id"]  # Already exists and still valid

so a second add_triple() carrying provenance (source_closet / source_file / source_drawer_id / adapter_name) or a valid_from the first writer didn't have still drops it silently. This PR backfills only the columns that are NULL, never overwrites a settled value, and lets a strictly higher confidence win. Scope is knowledge_graph.py + its tests.

CI is running fresh against the current base. Happy to split, narrow, or adjust the confidence rule if you'd prefer it stay untouched.

When add_triple is called a second time with the same (subject, predicate,
object) and valid_to IS NULL, the existing id was returned and every new
metadata value — valid_from, source_closet, source_file, source_drawer_id,
adapter_name, confidence — was silently dropped.

This is the exact 'a later adapter refines what an earlier one did not know'
pattern the RFC 002 §5.5 provenance fields were designed to support, and it
breaks the Incremental-only principle (CLAUDE.md): a second pass with better
information should be additive, never a silent no-op.

Policy on re-adding an existing open triple:
  - backfill NULL fields when a new non-NULL value is provided
  - never overwrite an already-populated field — explicit invalidate() is
    the only path to change settled values
  - confidence is the one exception: a strictly higher value replaces a
    weaker one so stronger evidence wins (weaker evidence is ignored)
  - the returned id is unchanged — idempotent contract preserved

Four regression tests cover the four cases: valid_from backfill, provenance
backfill, refusal to overwrite existing metadata, and monotonic confidence.
CONTRIBUTING.md line 73 calls out knowledge_graph.py coverage as an explicit
maintainer ask.
@arnoldwender
arnoldwender force-pushed the fix/kg-add-triple-backfill-null-metadata branch from c24a5d5 to b3208a2 Compare August 22, 2026 13:19
@arnoldwender

Copy link
Copy Markdown
Contributor Author

Rebased onto current develop (4bc0c43, post-v3.8.0) — clean, no conflicts, no logical changes. 303 commits had landed since the previous base; knowledge_graph.py gained 60 lines in that window but none of them touch this path.

Re-verified the bug is still live rather than just re-pinging: knowledge_graph.py:311 on develop is still the bare return existing["id"] # Already exists and still valid, so a second add_triple() for the same subject/predicate/object still discards valid_from, confidence and all four provenance columns when the first writer left them NULL.

Falsifier re-run on the rebased branch: reverting only the production hunk and keeping the tests turns 3 of the 4 new tests RED (backfills_null_valid_from, backfills_null_provenance, upgrades_confidence_only_when_higher). The fourth (does_not_overwrite_existing_metadata) is green either way by design — it guards the non-overwrite invariant, not the bug.

Full suite green locally on the rebased branch: 4472 passed, 31 skipped, ruff 0.16.1 clean.

@arnoldwender

Copy link
Copy Markdown
Contributor Author

@igorls — friendly ping on this one. It is the last of my April cohort still open; #1104, #2194, #2208 and #1140 all landed over the last two weeks, so I suspect this one just fell through the cracks rather than being held back.

Current state after today's rebase onto 4bc0c43 (post-v3.8.0): CI 9/9 green including test-windows and the new smoke check, mergeable_state: clean, 34 lines of production change confined to one branch of add_triple(), plus 4 tests.

No rush and no hard feelings if the answer is "not this shape" — happy to split it, shrink it, or close it if it is not worth the review slot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/kg Knowledge graph bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants