mirror of
https://github.com/semantica-agi/semantica.git
synced 2026-08-29 04:26:20 +00:00
Fix code review findings: metadata precedence and Mapping support
- get_lineage() aggregated metadata by iterating trace_lineage()'s BFS order and calling dict.update() on each entry, so ancestor metadata (now reachable via derived_from chains) could overwrite the queried entity's own metadata on conflicting keys. Reverse the iteration so the queried entity (always lineage_entries[0]) is applied last and wins, matching the documented "most recent entry's metadata takes precedence" intent. - track_entity()'s derived_from guard only accepted a concrete dict, silently ignoring other collections.abc.Mapping implementations (e.g. types.MappingProxyType). Switch the isinstance check to Mapping so any mapping-like metadata is honored. Addresses Qodo review findings on PR #741.
This commit is contained in:
+3
-1
@@ -26,7 +26,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
- `track_entity()`'s only auto-linking logic looked up `source` as if it were an existing entity's `entity_id`, so passing the same real URL/DOI as `source` for two conceptually linked entities (e.g. a document and a decision derived from it) never produced a parent link, leaving `get_lineage()` returning a chain of length 1
|
||||
- `metadata["derived_from"]` was preserved and echoed back in the output JSON but was never consulted by any linking or traversal code, so the caller's explicit relationship was silently inert
|
||||
- `track_entity()` now treats `metadata["derived_from"]` as an explicit parent link (unless `parent_entity_id` was already passed directly), so `InMemoryStorage.trace_lineage()`'s existing BFS over `parent_entity_id` picks it up for free
|
||||
- Added 7 regression/edge-case tests in `tests/provenance/test_manager.py` covering the happy path, explicit `parent_entity_id` precedence over `derived_from`, precedence over the `source`-as-known-entity-id fallback, a `derived_from` pointing at a never-tracked entity, non-string/empty-string `derived_from` values being ignored, a self-referencing `derived_from` not hanging traversal, and multi-hop `derived_from` chains, closing #735
|
||||
- `metadata["derived_from"]` is now recognized on any `collections.abc.Mapping`, not just a concrete `dict`, so e.g. `types.MappingProxyType` metadata still creates the parent link
|
||||
- `get_lineage()`'s metadata aggregation now applies the queried entity's own metadata last so it wins over ancestor metadata on conflicting keys, matching the documented "most recent entry's metadata takes precedence" behavior — previously `trace_lineage()`'s BFS order caused ancestor metadata (now reachable via `derived_from` chains) to silently overwrite the queried entity's own values
|
||||
- Added 9 regression/edge-case tests in `tests/provenance/test_manager.py` covering the happy path, explicit `parent_entity_id` precedence over `derived_from`, precedence over the `source`-as-known-entity-id fallback, a `derived_from` pointing at a never-tracked entity, non-string/empty-string `derived_from` values being ignored, a self-referencing `derived_from` not hanging traversal, multi-hop `derived_from` chains, metadata precedence between a queried entity and its ancestors, and non-`dict` `Mapping` metadata, closing #735
|
||||
|
||||
- **`InferenceResult.premises` always empty from `forward_chain`/`backward_chain`** (#739) by @Sameer6305
|
||||
- `_match_rule()` discarded matched facts and returned only instantiated conclusions, so `ExplanationGenerator` always produced empty premises lists regardless of which facts actually satisfied a rule, closing #733
|
||||
|
||||
@@ -25,6 +25,7 @@ License: MIT
|
||||
"""
|
||||
|
||||
from typing import Optional, List, Dict, Any
|
||||
from collections.abc import Mapping
|
||||
from datetime import datetime
|
||||
|
||||
from .schemas import ProvenanceEntry, SourceReference, PropertySource
|
||||
@@ -118,7 +119,7 @@ class ProvenanceManager:
|
||||
|
||||
# If caller declared an explicit parent via metadata, honor it
|
||||
# (unless parent_entity_id was already passed directly)
|
||||
if not parent_id and metadata and isinstance(metadata, dict):
|
||||
if not parent_id and metadata and isinstance(metadata, Mapping):
|
||||
derived_from = metadata.get("derived_from")
|
||||
if derived_from and isinstance(derived_from, str):
|
||||
parent_id = derived_from
|
||||
@@ -460,10 +461,14 @@ class ProvenanceManager:
|
||||
if not lineage_entries:
|
||||
return {}
|
||||
|
||||
# Aggregate metadata from all lineage entries
|
||||
# Most recent entry's metadata takes precedence
|
||||
# Aggregate metadata from all lineage entries.
|
||||
# trace_lineage() is a BFS starting at entity_id, so lineage_entries[0]
|
||||
# is always the queried entity itself, followed by its ancestors
|
||||
# (parent, grandparent, ...). Apply ancestors first and the queried
|
||||
# entity last so its own keys win on conflict, matching the intent
|
||||
# that the "most recent"/current entity's metadata takes precedence.
|
||||
aggregated_metadata = {}
|
||||
for entry in lineage_entries:
|
||||
for entry in reversed(lineage_entries):
|
||||
if entry.metadata:
|
||||
meta = entry.metadata
|
||||
if isinstance(meta, str):
|
||||
|
||||
@@ -248,6 +248,48 @@ class TestProvenanceManager:
|
||||
|
||||
assert entry.parent_entity_id is None
|
||||
|
||||
def test_get_lineage_metadata_prefers_queried_entity_over_ancestors(self):
|
||||
"""Aggregated lineage metadata should let the queried entity's own
|
||||
values win over ancestor values on conflicting keys, matching the
|
||||
documented "most recent entry's metadata takes precedence" intent."""
|
||||
prov_mgr = ProvenanceManager()
|
||||
|
||||
prov_mgr.track_entity(
|
||||
entity_id="ancestor",
|
||||
source="doc_1",
|
||||
metadata={"status": "draft", "shared_only_on_ancestor": True},
|
||||
)
|
||||
prov_mgr.track_entity(
|
||||
entity_id="descendant",
|
||||
source="doc_1",
|
||||
metadata={"status": "final", "derived_from": "ancestor"},
|
||||
)
|
||||
|
||||
lineage = prov_mgr.get_lineage("descendant")
|
||||
|
||||
assert lineage["metadata"]["status"] == "final"
|
||||
assert lineage["metadata"]["shared_only_on_ancestor"] is True
|
||||
|
||||
def test_derived_from_accepts_non_dict_mapping(self):
|
||||
"""metadata['derived_from'] should be honored for any Mapping
|
||||
implementation, not just a concrete dict (e.g. types.MappingProxyType
|
||||
or a custom collections.abc.Mapping)."""
|
||||
from types import MappingProxyType
|
||||
|
||||
prov_mgr = ProvenanceManager()
|
||||
|
||||
prov_mgr.track_entity(entity_id="mapping_parent", source="doc_1")
|
||||
entry = prov_mgr.track_entity(
|
||||
entity_id="mapping_child",
|
||||
source="doc_1",
|
||||
metadata=MappingProxyType({"derived_from": "mapping_parent"}),
|
||||
)
|
||||
|
||||
assert entry.parent_entity_id == "mapping_parent"
|
||||
|
||||
lineage = prov_mgr.get_lineage("mapping_child")
|
||||
assert lineage["entity_count"] == 2
|
||||
|
||||
def test_batch_entity_tracking(self):
|
||||
"""Test batch entity tracking."""
|
||||
prov_mgr = ProvenanceManager()
|
||||
|
||||
Reference in New Issue
Block a user