From fdabaf52d673f3db0f7f9142f39be42d4b6ad000 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:47:39 -0700 Subject: [PATCH 01/22] fix: HttpLedgerBackend routed events to a phantom empty-name model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ledger(HttpLedgerBackend(url)) — the documented remote-backend path — corrupted the remote ledger three ways: - append_snapshot POSTed /record with model_name: "" (a comment claimed it was resolved server-side; it was not), so the server auto-registered a model literally named "" and attached every recorded event to it. It now resolves the real model name from the hash-to-name cache and raises ModelNotFoundError for unresolvable hashes instead of corrupting the remote inventory. Responses are raise_for_status()-checked. - register() double-logged the registration: save_model's POST /record already logs the registered event server-side, and the SDK's follow-up append_snapshot(registered) posted it again. The backend now skips that redundant post, and the record tool itself logs exactly one registered event per new model (Ledger.register grew an optional payload= merge so the caller's registration payload still lands on the single event). Re-registering an existing model is now idempotent (is_new_model=False, pointing at the original registration event) instead of appending duplicate registered events. - get_snapshot() always returned None. It now serves appended snapshots from a client-side cache and reconstructs older ones from GET /changelog (snapshot hashes are content-derived, so rebuilt snapshots recompute the same identity the server holds). Defense in depth: RecordInput.model_name now requires min_length=1, so old clients that still POST an empty model_name get a 422 instead of silently corrupting the ledger. New integration suite drives Ledger over HttpLedgerBackend against the real create_app() handlers (TestClient, not mocks): no phantom model, exactly one registered event, events attach to the named model, get_snapshot round-trips, and empty model_name is rejected. Co-Authored-By: Claude Fable 5 --- src/model_ledger/backends/http.py | 46 +++++- src/model_ledger/sdk/ledger.py | 23 ++- src/model_ledger/tools/record.py | 59 +++++-- src/model_ledger/tools/schemas.py | 2 +- tests/test_backends/test_http_backend.py | 8 + .../test_http_ledger_integration.py | 155 ++++++++++++++++++ tests/test_tools/test_investigate.py | 3 +- 7 files changed, 266 insertions(+), 30 deletions(-) create mode 100644 tests/test_backends/test_http_ledger_integration.py diff --git a/src/model_ledger/backends/http.py b/src/model_ledger/backends/http.py index 3df6afc..01cebde 100644 --- a/src/model_ledger/backends/http.py +++ b/src/model_ledger/backends/http.py @@ -37,6 +37,12 @@ def __init__(self, base_url: str, headers: dict[str, str] | None = None) -> None # cannot reverse-resolve on writes like set_tag. Populated lazily # whenever a name lookup succeeds (get_model_by_name, save_model, etc.). self._hash_to_name: dict[str, str] = {} + # Hashes whose registration event was already logged server-side by + # save_model's POST /record — append_snapshot must not repost it. + self._registered_hashes: set[str] = set() + # Snapshots appended through this backend, kept for get_snapshot() + # round-trips (the REST API has no snapshot-by-hash endpoint). + self._snapshot_cache: dict[str, Snapshot] = {} # ── Models ── @@ -71,6 +77,10 @@ def save_model(self, model: ModelRef) -> None: ) model.model_hash = server_hash self._hash_to_name[server_hash] = model.name + # The server's /record tool logs the "registered" event as part of + # registration — remember that so the SDK's follow-up + # append_snapshot(registered) is not posted a second time. + self._registered_hashes.add(server_hash) def get_model(self, model_hash: str) -> ModelRef | None: name = self._hash_to_name.get(model_hash) @@ -121,7 +131,7 @@ def list_models(self, **filters: str) -> list[ModelRef]: return models def update_model(self, model: ModelRef) -> None: - self._client.post( + resp = self._client.post( "/record", json={ "model_name": model.name, @@ -129,22 +139,50 @@ def update_model(self, model: ModelRef) -> None: "payload": {"status": model.status}, }, ) + resp.raise_for_status() # ── Snapshots ── + def _resolve_name(self, model_hash: str) -> str: + """Resolve a model_hash to its server-side name, or fail loudly.""" + name = self._hash_to_name.get(model_hash) + if name is not None: + return name + model = self.get_model(model_hash) + if model is not None: + return model.name + raise ModelNotFoundError(model_hash) + def append_snapshot(self, snapshot: Snapshot) -> None: - self._client.post( + if snapshot.event_type == "registered" and snapshot.model_hash in self._registered_hashes: + # save_model's POST /record already logged this registration + # server-side; posting it again would duplicate the event. + self._registered_hashes.discard(snapshot.model_hash) + return + resp = self._client.post( "/record", json={ - "model_name": "", # resolved server-side + "model_name": self._resolve_name(snapshot.model_hash), "event": snapshot.event_type, "payload": snapshot.payload, "actor": snapshot.actor, }, ) + resp.raise_for_status() + self._snapshot_cache[snapshot.snapshot_hash] = snapshot def get_snapshot(self, snapshot_hash: str) -> Snapshot | None: - # Not directly exposed via REST — return None + cached = self._snapshot_cache.get(snapshot_hash) + if cached is not None: + return cached + # The REST API has no snapshot-by-hash endpoint. Snapshot hashes are + # content-derived (model_hash + timestamp + payload), so snapshots + # rebuilt from GET /changelog recompute the same identity the server + # holds — scan the changelog of every model resolved so far. + for model_hash in list(self._hash_to_name): + for snap in self.list_snapshots(model_hash): + if snap.snapshot_hash == snapshot_hash: + return snap return None def list_snapshots(self, model_hash: str, **filters: str) -> list[Snapshot]: diff --git a/src/model_ledger/sdk/ledger.py b/src/model_ledger/sdk/ledger.py index aab4b14..b52fa04 100644 --- a/src/model_ledger/sdk/ledger.py +++ b/src/model_ledger/sdk/ledger.py @@ -146,7 +146,13 @@ def register( status: str = "active", actor: str = "system", metadata: dict[str, Any] | None = None, + payload: dict[str, Any] | None = None, ) -> ModelRef: + """Register a model, logging exactly one ``registered`` event. + + ``payload`` entries are merged into the registration event's payload + on top of the canonical identity fields (caller keys win). + """ if name in self._name_cache: return self._name_cache[name] if not self._cache_complete: @@ -165,18 +171,21 @@ def register( metadata=metadata or {}, ) self._backend.save_model(model) + event_payload: dict[str, Any] = { + "name": name, + "owner": owner, + "tier": tier, + "purpose": purpose, + "model_origin": model_origin, + } + if payload: + event_payload.update(payload) self._backend.append_snapshot( Snapshot( model_hash=model.model_hash, actor=actor, event_type="registered", - payload={ - "name": name, - "owner": owner, - "tier": tier, - "purpose": purpose, - "model_origin": model_origin, - }, + payload=event_payload, ) ) self._name_cache[name] = model diff --git a/src/model_ledger/tools/record.py b/src/model_ledger/tools/record.py index 1714170..7bcf53d 100644 --- a/src/model_ledger/tools/record.py +++ b/src/model_ledger/tools/record.py @@ -10,7 +10,9 @@ def record(input: RecordInput, ledger: Ledger) -> RecordOutput: """Register a new model or record an event on an existing model. When ``input.event == "registered"``, creates the model via - ``ledger.register()`` then logs the registration event. + ``ledger.register()``, which logs exactly one ``registered`` event + carrying ``input.payload``. Re-registering an existing model is + idempotent: no duplicate event is appended. Otherwise, looks up the existing model and appends the event. Raises: @@ -18,26 +20,49 @@ def record(input: RecordInput, ledger: Ledger) -> RecordOutput: is not ``"registered"``. """ if input.event == "registered": - model = ledger.register( - name=input.model_name, - owner=input.owner or "unknown", - model_type=input.model_type or "unknown", - tier="unclassified", - purpose=input.purpose or "", - actor=input.actor, - ) - snapshot = ledger.record( - model, - event="registered", - payload=input.payload, - actor=input.actor, - ) + existing = ledger._backend.get_model_by_name(input.model_name) + if existing is None: + model = ledger.register( + name=input.model_name, + owner=input.owner or "unknown", + model_type=input.model_type or "unknown", + tier="unclassified", + purpose=input.purpose or "", + actor=input.actor, + payload=input.payload or None, + ) + # register() appended the single canonical "registered" snapshot. + snapshot = ledger._backend.latest_snapshot(model.model_hash) + if snapshot is None: # pragma: no cover — register always logs one + raise RuntimeError("register() did not log a registration event") + return RecordOutput( + model_name=input.model_name, + model_hash=model.model_hash, + event_id=snapshot.snapshot_hash, + timestamp=snapshot.timestamp, + is_new_model=True, + ) + + # Idempotent re-registration: point at the existing registration + # event instead of appending a duplicate. + registered = ledger._backend.list_snapshots(existing.model_hash, event_type="registered") + if registered: + snapshot = max(registered, key=lambda s: s.timestamp) + else: + # Model exists but has no registration event (e.g. imported + # data) — log one now so the audit trail is complete. + snapshot = ledger.record( + existing, + event="registered", + payload=input.payload, + actor=input.actor, + ) return RecordOutput( model_name=input.model_name, - model_hash=model.model_hash, + model_hash=existing.model_hash, event_id=snapshot.snapshot_hash, timestamp=snapshot.timestamp, - is_new_model=True, + is_new_model=False, ) # Non-registration event: model must already exist diff --git a/src/model_ledger/tools/schemas.py b/src/model_ledger/tools/schemas.py index a12e4fe..d10ed0d 100644 --- a/src/model_ledger/tools/schemas.py +++ b/src/model_ledger/tools/schemas.py @@ -61,7 +61,7 @@ class DependencyNode(BaseModel): class RecordInput(BaseModel): """Input for the record tool — log an event against a model.""" - model_name: str + model_name: str = Field(min_length=1) event: str payload: dict[str, Any] = Field(default_factory=dict) actor: str = "user" diff --git a/tests/test_backends/test_http_backend.py b/tests/test_backends/test_http_backend.py index 8ac3f77..19f363e 100644 --- a/tests/test_backends/test_http_backend.py +++ b/tests/test_backends/test_http_backend.py @@ -29,6 +29,8 @@ def http_backend(): backend._base_url = "http://testserver" backend._client = TestClient(app) backend._hash_to_name = {} + backend._registered_hashes = set() + backend._snapshot_cache = {} yield backend backend._client.close() @@ -234,6 +236,8 @@ def test_tag_flow_survives_fresh_backend_instance(self, http_backend): fresh_backend._base_url = http_backend._base_url fresh_backend._client = http_backend._client fresh_backend._hash_to_name = {} + fresh_backend._registered_hashes = set() + fresh_backend._snapshot_cache = {} ledger = Ledger(backend=fresh_backend) created = ledger.tag("scoring-model", "v1.0") @@ -264,6 +268,8 @@ def _always_500(request: httpx.Request) -> httpx.Response: transport=httpx.MockTransport(_always_500), ) backend._hash_to_name = {} + backend._registered_hashes = set() + backend._snapshot_cache = {} ref = ModelRef( name="credit-scorecard", @@ -301,6 +307,8 @@ def _missing_hash(request: httpx.Request) -> httpx.Response: transport=httpx.MockTransport(_missing_hash), ) backend._hash_to_name = {} + backend._registered_hashes = set() + backend._snapshot_cache = {} ref = ModelRef( name="credit-scorecard", diff --git a/tests/test_backends/test_http_ledger_integration.py b/tests/test_backends/test_http_ledger_integration.py new file mode 100644 index 0000000..c2e71a1 --- /dev/null +++ b/tests/test_backends/test_http_ledger_integration.py @@ -0,0 +1,155 @@ +# tests/test_backends/test_http_ledger_integration.py +"""End-to-end: ``Ledger(HttpLedgerBackend(url))`` against real REST handlers. + +Regression tests for the remote-SDK path advertised in +docs/guides/backends.md. The mocked unit tests cannot catch payload-shape +mismatches between the SDK backend and the server's ``/record`` tool — +previously ``append_snapshot`` posted ``model_name: ""``, so the server +auto-registered a phantom model named ``""`` and attached every event to +it, while ``register()`` double-logged the registration event. + +These tests drive the real ``create_app()`` handlers in-process via +Starlette's TestClient (a sync httpx.Client subclass), NOT mocks. +""" + +from __future__ import annotations + +import pytest + +pytest.importorskip("httpx") +pytest.importorskip("fastapi") + +from fastapi.testclient import TestClient + +from model_ledger.backends.http import HttpLedgerBackend +from model_ledger.core.exceptions import ModelNotFoundError +from model_ledger.core.ledger_models import Snapshot +from model_ledger.rest.app import create_app +from model_ledger.sdk.ledger import Ledger + + +@pytest.fixture +def app(): + return create_app() + + +@pytest.fixture +def backend(app): + """A fully-constructed HttpLedgerBackend whose transport is the real app.""" + b = HttpLedgerBackend("http://testserver") + b._client.close() + b._client = TestClient(app) + yield b + b._client.close() + + +@pytest.fixture +def server_client(app): + """Independent client for asserting server-side state.""" + return TestClient(app) + + +@pytest.fixture +def ledger(backend): + return Ledger(backend=backend) + + +def _register(ledger): + return ledger.register( + name="remote-model", + owner="risk-team", + model_type="ml_model", + tier="low", + purpose="scoring", + ) + + +class TestRegisterOverHttp: + def test_register_creates_no_phantom_model(self, ledger, server_client): + _register(ledger) + + names = [m["name"] for m in server_client.get("/query").json()["models"]] + assert "" not in names + assert names == ["remote-model"] + + def test_register_logs_exactly_one_registered_event(self, ledger, server_client): + _register(ledger) + + resp = server_client.get("/changelog", params={"model_name": "remote-model"}) + events = resp.json()["events"] + registered = [e for e in events if e["event_type"] == "registered"] + assert len(registered) == 1 + + +class TestRecordOverHttp: + def test_event_attaches_to_named_model(self, ledger, server_client): + _register(ledger) + ledger.record("remote-model", event="deployed", payload={"env": "prod"}, actor="ci") + + events = server_client.get("/changelog").json()["events"] + deployed = [e for e in events if e["event_type"] == "deployed"] + assert len(deployed) == 1 + assert deployed[0]["model_name"] == "remote-model" + assert deployed[0]["payload"] == {"env": "prod"} + + # No phantom model appeared as a side effect of recording. + names = [m["name"] for m in server_client.get("/query").json()["models"]] + assert names == ["remote-model"] + + def test_ledger_list_round_trip(self, ledger): + _register(ledger) + ledger.record("remote-model", event="deployed", payload={}, actor="ci") + + assert [m.name for m in ledger.list()] == ["remote-model"] + + def test_append_snapshot_unknown_hash_fails_loudly(self, backend): + """An unresolvable model_hash must raise, not corrupt the remote ledger.""" + snap = Snapshot( + model_hash="0" * 32, + actor="ci", + event_type="deployed", + payload={}, + ) + with pytest.raises(ModelNotFoundError): + backend.append_snapshot(snap) + + +class TestGetSnapshotOverHttp: + def test_get_snapshot_round_trips(self, ledger, backend): + model = _register(ledger) + ledger.record("remote-model", event="deployed", payload={"env": "prod"}, actor="ci") + + snaps = backend.list_snapshots(model.model_hash) + assert snaps # registration + deployed visible via changelog + for snap in snaps: + got = backend.get_snapshot(snap.snapshot_hash) + assert got is not None + assert got.snapshot_hash == snap.snapshot_hash + assert got.event_type == snap.event_type + assert got.model_hash == model.model_hash + + def test_get_snapshot_unknown_hash_returns_none(self, backend): + assert backend.get_snapshot("f" * 32) is None + + +class TestServerRejectsEmptyModelName: + """Defense in depth: old clients that still post ``model_name: ""`` + must fail loudly instead of silently corrupting the ledger.""" + + def test_record_empty_model_name_is_rejected(self, server_client): + resp = server_client.post( + "/record", + json={"model_name": "", "event": "deployed", "actor": "old-client"}, + ) + assert resp.status_code == 422 + + # Nothing was registered. + assert server_client.get("/query").json()["total"] == 0 + + def test_record_empty_model_name_registered_is_rejected(self, server_client): + resp = server_client.post( + "/record", + json={"model_name": "", "event": "registered", "actor": "old-client"}, + ) + assert resp.status_code == 422 + assert server_client.get("/query").json()["total"] == 0 diff --git a/tests/test_tools/test_investigate.py b/tests/test_tools/test_investigate.py index e646304..1caf3fd 100644 --- a/tests/test_tools/test_investigate.py +++ b/tests/test_tools/test_investigate.py @@ -87,7 +87,8 @@ def test_total_events_counted(self, ledger): result = investigate(InvestigateInput(model_name="fraud_scoring"), ledger) - assert result.total_events >= 3 + # Registration logs exactly one event (previously double-fired). + assert result.total_events == 2 def test_days_since_last_event(self, ledger): record( From 0c2156424cd3d03b1cf543c5c98187c763f548eb Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:49:38 -0700 Subject: [PATCH 02/22] fix: query silently ignored the platform filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QueryInput, the REST /query endpoint, and the MCP query tool all advertise a platform parameter, but query() never read input.platform — any platform value returned the full inventory. Silently-wrong results for a documented filter. Platform is derived from snapshot data (newest discovered payload, then snapshot source), not a column on the model row, so it cannot ride the structural list_models pushdown. query() now resolves platforms for all structurally-matched models in one batched dispatch (batch_platforms — a single SQL round trip on SQL-backed backends) and filters/paginates on the result. An unknown platform now returns 0 models, not 35k. Adds platform filter tests (match, zero-match, text combination, pagination) and a schema-vs-implementation parity test that fails if QueryInput ever grows a filter field query() does not consume. Co-Authored-By: Claude Fable 5 --- src/model_ledger/tools/query.py | 78 ++++++++++++++++------ tests/test_tools/test_query.py | 111 ++++++++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 18 deletions(-) diff --git a/src/model_ledger/tools/query.py b/src/model_ledger/tools/query.py index 586fb12..dc3143e 100644 --- a/src/model_ledger/tools/query.py +++ b/src/model_ledger/tools/query.py @@ -40,11 +40,35 @@ def _model_to_summary(model: ModelRef, ledger: Ledger) -> ModelSummary: ) +def _summarize(models: list[ModelRef], ledger: Ledger) -> list[ModelSummary]: + """Build enriched ModelSummary rows via one batched backend dispatch.""" + backend = ledger._backend + model_hashes = [m.model_hash for m in models] + if hasattr(backend, "model_summaries"): + enrichment = backend.model_summaries(model_hashes) + else: + enrichment = batch_fallbacks.model_summaries(backend, model_hashes) + return [ + ModelSummary( + name=m.name, + owner=m.owner, + model_type=m.model_type, + status=m.status, + platform=enrichment.get(m.model_hash, {}).get("platform"), + last_event=enrichment.get(m.model_hash, {}).get("last_event"), + event_count=enrichment.get(m.model_hash, {}).get("event_count", 0), + ) + for m in models + ] + + def query(input: QueryInput, ledger: Ledger) -> QueryOutput: """Search and filter the model inventory with pagination. Pushes limit, offset, and text filters to the backend when supported - (e.g., Snowflake SQL) to avoid fetching all rows. + (e.g., Snowflake SQL) to avoid fetching all rows. The ``platform`` + filter is derived from snapshot data (``discovered`` payload, then + snapshot source), so it is resolved in batch and applied here. """ filters: dict[str, str] = {} if input.model_type is not None: @@ -54,6 +78,9 @@ def query(input: QueryInput, ledger: Ledger) -> QueryOutput: if input.status is not None: filters["status"] = input.status + if input.platform is not None: + return _query_by_platform(input, ledger, filters) + count_filters = dict(filters) if input.text: count_filters["text"] = input.text @@ -81,22 +108,37 @@ def query(input: QueryInput, ledger: Ledger) -> QueryOutput: has_more = (input.offset + input.limit) < total - model_hashes = [m.model_hash for m in models] - if hasattr(backend, "model_summaries"): - enrichment = backend.model_summaries(model_hashes) + return QueryOutput(total=total, models=_summarize(models, ledger), has_more=has_more) + + +def _query_by_platform(input: QueryInput, ledger: Ledger, filters: dict[str, str]) -> QueryOutput: + """Apply the platform filter over structurally-matched models. + + Platform is not a column on the model row — it is resolved from each + model's snapshots (``batch_platforms`` pushes this down in SQL-backed + backends) — so filtering and pagination happen here rather than in + ``list_models``. + """ + backend = ledger._backend + + models_all = ledger.list(**filters) + if input.text: + text_lower = input.text.lower() + models_all = [ + m + for m in models_all + if text_lower in m.name.lower() or text_lower in (m.purpose or "").lower() + ] + + model_hashes = [m.model_hash for m in models_all] + if hasattr(backend, "batch_platforms"): + platforms = backend.batch_platforms(model_hashes) else: - enrichment = batch_fallbacks.model_summaries(backend, model_hashes) - summaries = [ - ModelSummary( - name=m.name, - owner=m.owner, - model_type=m.model_type, - status=m.status, - platform=enrichment.get(m.model_hash, {}).get("platform"), - last_event=enrichment.get(m.model_hash, {}).get("last_event"), - event_count=enrichment.get(m.model_hash, {}).get("event_count", 0), - ) - for m in models - ] + platforms = batch_fallbacks.batch_platforms(backend, model_hashes) + + matched = [m for m in models_all if platforms.get(m.model_hash) == input.platform] + total = len(matched) + page = matched[input.offset : input.offset + input.limit] + has_more = (input.offset + input.limit) < total - return QueryOutput(total=total, models=summaries, has_more=has_more) + return QueryOutput(total=total, models=_summarize(page, ledger), has_more=has_more) diff --git a/tests/test_tools/test_query.py b/tests/test_tools/test_query.py index ec35a65..776d1f5 100644 --- a/tests/test_tools/test_query.py +++ b/tests/test_tools/test_query.py @@ -278,3 +278,114 @@ def test_summary_platform_from_source(self, ledger): summary = _model_to_summary(model, ledger) assert summary.platform == "mlflow" + + +class TestQueryFilterByPlatform: + """Filter by platform — derived from snapshots, not a model column.""" + + @pytest.fixture + def platform_ledger(self, ledger): + for name, platform in [ + ("mlflow-model", "mlflow"), + ("workflow-model", "workflow-engine"), + ("second-mlflow-model", "mlflow"), + ]: + _register(ledger, name, owner="data-team") + ledger.record( + name, + event="discovered", + payload={"platform": platform}, + actor="connector", + source=platform, + ) + _register(ledger, "no-platform-model", owner="data-team") + return ledger + + def test_filter_platform(self, platform_ledger): + result = query(QueryInput(platform="mlflow"), platform_ledger) + + assert result.total == 2 + assert {m.name for m in result.models} == {"mlflow-model", "second-mlflow-model"} + assert all(m.platform == "mlflow" for m in result.models) + + def test_filter_platform_no_match_returns_zero(self, platform_ledger): + """An unknown platform must return 0 results, not the full inventory.""" + result = query(QueryInput(platform="no_such_platform"), platform_ledger) + + assert result.total == 0 + assert result.models == [] + assert result.has_more is False + + def test_platform_combines_with_text(self, platform_ledger): + result = query(QueryInput(platform="mlflow", text="second"), platform_ledger) + + assert result.total == 1 + assert result.models[0].name == "second-mlflow-model" + + def test_platform_with_pagination(self, platform_ledger): + result = query(QueryInput(platform="mlflow", limit=1), platform_ledger) + + assert result.total == 2 + assert len(result.models) == 1 + assert result.has_more is True + + page2 = query(QueryInput(platform="mlflow", limit=1, offset=1), platform_ledger) + assert len(page2.models) == 1 + assert page2.models[0].name != result.models[0].name + assert page2.has_more is False + + +class TestQueryInputParity: + """Every filter field advertised on QueryInput must be consumed by query().""" + + def test_every_advertised_filter_narrows_results(self, ledger): + _register( + ledger, + "alpha-model", + owner="team-a", + model_type="ml_model", + purpose="alpha purpose", + ) + _register( + ledger, + "beta-rules", + owner="team-b", + model_type="heuristic", + purpose="beta purpose", + ) + ledger.record( + "alpha-model", + event="discovered", + payload={"platform": "mlflow"}, + actor="connector", + source="mlflow", + ) + ledger.record( + "beta-rules", + event="discovered", + payload={"platform": "workflow-engine"}, + actor="connector", + source="workflow-engine", + ) + beta = ledger.get("beta-rules") + beta.status = "deprecated" + ledger._backend.update_model(beta) + + matching = { + "text": "alpha", + "platform": "mlflow", + "model_type": "ml_model", + "owner": "team-a", + "status": "active", + } + pagination = {"limit", "offset"} + # Schema-vs-implementation parity: fail if QueryInput grows a filter + # field this test does not exercise. + assert set(QueryInput.model_fields) == set(matching) | pagination + + for field, value in matching.items(): + result = query(QueryInput(**{field: value}), ledger) + assert result.total == 1, f"QueryInput.{field} is not consumed by query()" + assert result.models[0].name == "alpha-model", ( + f"QueryInput.{field} did not narrow results" + ) From 31506532f8d35498b49560a0af0b60396653ad0b Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:51:41 -0700 Subject: [PATCH 03/22] fix: registry dispatched xgboost sklearn-API models to the sklearn introspector Entry points load sorted by name (lightgbm < sklearn < xgboost) and find() returns the first can_handle match. XGBClassifier & friends are isinstance of sklearn.BaseEstimator, so the generic sklearn introspector claimed them and xgboost-specific extraction was lost (result carried introspector='sklearn', framework='scikit-learn'). LightGBM only escaped because 'lightgbm' happens to sort before 'sklearn'. SklearnIntrospector.can_handle now rejects estimators whose class comes from a wrapper framework with a dedicated introspector (xgboost.*, lightgbm.* modules), making dispatch independent of registry order. New dispatch tests (importorskip'd on the ML libs): XGBClassifier -> xgboost, LGBMClassifier -> lightgbm, plain LogisticRegression -> sklearn, and an order-independence check on can_handle itself. Co-Authored-By: Claude Fable 5 --- src/model_ledger/introspect/sklearn.py | 11 +++- tests/test_introspect/test_dispatch.py | 82 ++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 tests/test_introspect/test_dispatch.py diff --git a/src/model_ledger/introspect/sklearn.py b/src/model_ledger/introspect/sklearn.py index 3520328..40c7ca6 100644 --- a/src/model_ledger/introspect/sklearn.py +++ b/src/model_ledger/introspect/sklearn.py @@ -14,13 +14,20 @@ class SklearnIntrospector: name = "sklearn" + # Frameworks whose sklearn-API wrappers subclass BaseEstimator but have + # dedicated introspectors. The generic sklearn introspector must defer to + # those regardless of registry iteration order. + _WRAPPER_MODULES = frozenset({"xgboost", "lightgbm"}) + def can_handle(self, obj: Any) -> bool: try: from sklearn.base import BaseEstimator - - return isinstance(obj, BaseEstimator) except ImportError: return False + if not isinstance(obj, BaseEstimator): + return False + root_module = (type(obj).__module__ or "").split(".", 1)[0] + return root_module not in self._WRAPPER_MODULES def introspect(self, obj: Any) -> IntrospectionResult: from sklearn.pipeline import Pipeline diff --git a/tests/test_introspect/test_dispatch.py b/tests/test_introspect/test_dispatch.py new file mode 100644 index 0000000..e721a15 --- /dev/null +++ b/tests/test_introspect/test_dispatch.py @@ -0,0 +1,82 @@ +# tests/test_introspect/test_dispatch.py +"""Registry dispatch: framework-specific introspectors win over generic sklearn. + +Entry points load ``sorted(key=name)`` (lightgbm < sklearn < xgboost) and +``find()`` returns the first ``can_handle`` match. XGBoost/LightGBM +sklearn-API wrappers are ``isinstance`` of ``sklearn.BaseEstimator``, so +without an explicit rejection in ``SklearnIntrospector.can_handle`` the +generic introspector shadows the xgboost one (and lightgbm only escaped by +alphabetical luck). +""" + +from __future__ import annotations + +import pytest + +from model_ledger.introspect.registry import get_registry, reset_registry + +np = pytest.importorskip("numpy") + + +@pytest.fixture(autouse=True) +def fresh_registry(): + reset_registry() + yield + reset_registry() + + +def _training_data(): + X = np.array([[1.0, 2.0], [3.0, 4.0], [5.0, 6.0], [7.0, 8.0]]) + y = np.array([0, 1, 0, 1]) + return X, y + + +def test_xgboost_sklearn_api_dispatches_to_xgboost(): + xgb = pytest.importorskip("xgboost") + pytest.importorskip("sklearn") + + X, y = _training_data() + model = xgb.XGBClassifier(n_estimators=2, max_depth=2).fit(X, y) + + intro = get_registry().find(model) + assert intro.name == "xgboost" + + result = intro.introspect(model) + assert result.introspector == "xgboost" + assert result.framework == "xgboost" + + +def test_lightgbm_sklearn_api_dispatches_to_lightgbm(): + lgb = pytest.importorskip("lightgbm") + pytest.importorskip("sklearn") + + X, y = _training_data() + model = lgb.LGBMClassifier(n_estimators=2, min_child_samples=1).fit(X, y) + + intro = get_registry().find(model) + assert intro.name == "lightgbm" + + +def test_plain_sklearn_estimator_still_dispatches_to_sklearn(): + sklearn = pytest.importorskip("sklearn") # noqa: F841 + from sklearn.linear_model import LogisticRegression + + X, y = _training_data() + model = LogisticRegression().fit(X, y) + + intro = get_registry().find(model) + assert intro.name == "sklearn" + + +def test_sklearn_can_handle_rejects_wrapper_modules_regardless_of_order(): + """Order-independence: even if a new introspector name sorted the sklearn + introspector first, its can_handle must refuse wrapper-library objects.""" + pytest.importorskip("sklearn") + xgb = pytest.importorskip("xgboost") + + from model_ledger.introspect.sklearn import SklearnIntrospector + + X, y = _training_data() + model = xgb.XGBClassifier(n_estimators=2, max_depth=2).fit(X, y) + + assert SklearnIntrospector().can_handle(model) is False From 3fd3f7c5dcfdd84536153337fc8a3a8cf49a6a38 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:53:10 -0700 Subject: [PATCH 04/22] fix: SQL lineage extraction silently dropped unqualified table names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extract_tables_from_sql / extract_write_tables required 1-2 dotted segments, so INSERT INTO feature_store SELECT * FROM raw_events parsed to zero reads and zero writes — sql_connector(sql_column=...) produced zero lineage with no warning. The extract_write_tables docstring example (FROM source) was itself unextractable by its sibling function. Both extractors now accept bare (0-dot) identifiers, filtered against a small keyword blocklist so subqueries, table functions, and row sources (SELECT, LATERAL, TABLE(...), VALUES, ...) are not mistaken for tables. Qualified names are never keyword-filtered. INSERT OVERWRITE is also recognized instead of capturing OVERWRITE as a table name. Co-Authored-By: Claude Fable 5 --- src/model_ledger/adapters/sql.py | 51 ++++++++++++++++++++++++++++---- tests/test_adapters/test_sql.py | 39 ++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/src/model_ledger/adapters/sql.py b/src/model_ledger/adapters/sql.py index ea267af..6dccd7b 100644 --- a/src/model_ledger/adapters/sql.py +++ b/src/model_ledger/adapters/sql.py @@ -9,23 +9,58 @@ import re +# Bare (unqualified) identifiers that can follow FROM/JOIN/INTO in valid SQL +# without naming a table — subqueries, table functions, literal row sources, +# and statement keywords. Qualified names (with dots) are never filtered. +_SQL_NON_TABLE_KEYWORDS = frozenset( + { + "select", + "lateral", + "unnest", + "table", + "values", + "dual", + "generator", + "into", + "overwrite", + "if", + "not", + "exists", + "or", + "replace", + } +) + +_TABLE_IDENTIFIER = r"([a-zA-Z_][a-zA-Z0-9_]*(?:\.[a-zA-Z_][a-zA-Z0-9_]*){0,2})" + + +def _is_table_name(identifier: str) -> bool: + """True unless a bare identifier is really a SQL keyword.""" + return "." in identifier or identifier.lower() not in _SQL_NON_TABLE_KEYWORDS + def extract_tables_from_sql(sql: str | None) -> list[str]: """Extract table names from SQL FROM and JOIN clauses. + Handles fully-qualified (``db.schema.table``), schema-qualified + (``schema.table``), and bare (``table``) identifiers. Returns deduplicated list preserving first-seen order. Example: >>> extract_tables_from_sql("SELECT * FROM schema.table1 JOIN schema.table2 ON 1=1") ['schema.table1', 'schema.table2'] + >>> extract_tables_from_sql("SELECT * FROM raw_events") + ['raw_events'] """ if not sql: return [] - pattern = r"(?:FROM|JOIN)\s+([a-zA-Z_][a-zA-Z0-9_]*(?:\.[a-zA-Z_][a-zA-Z0-9_]*){1,2})" + pattern = rf"(?:FROM|JOIN)\s+{_TABLE_IDENTIFIER}" matches = re.findall(pattern, sql, re.IGNORECASE) seen: set[str] = set() result: list[str] = [] for table in matches: + if not _is_table_name(table): + continue lower = table.lower() if lower not in seen: seen.add(lower) @@ -36,24 +71,28 @@ def extract_tables_from_sql(sql: str | None) -> list[str]: def extract_write_tables(sql: str | None) -> list[str]: """Extract tables that a SQL statement writes to. - Handles INSERT INTO, CREATE [OR REPLACE] TABLE, MERGE INTO. + Handles INSERT INTO, CREATE [OR REPLACE] TABLE, MERGE INTO, with + qualified or bare table identifiers. Example: >>> extract_write_tables("INSERT INTO schema.output SELECT * FROM source") ['schema.output'] + >>> extract_write_tables("INSERT INTO feature_store SELECT * FROM raw_events") + ['feature_store'] """ if not sql: return [] - table_pattern = r"([a-zA-Z_][a-zA-Z0-9_]*(?:\.[a-zA-Z_][a-zA-Z0-9_]*){1,2})" results: list[str] = [] seen: set[str] = set() for pattern in [ - rf"INSERT\s+(?:INTO\s+)?{table_pattern}", - rf"CREATE\s+(?:OR\s+REPLACE\s+)?TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?{table_pattern}", - rf"MERGE\s+INTO\s+{table_pattern}", + rf"INSERT\s+(?:OVERWRITE\s+)?(?:INTO\s+)?{_TABLE_IDENTIFIER}", + rf"CREATE\s+(?:OR\s+REPLACE\s+)?TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?{_TABLE_IDENTIFIER}", + rf"MERGE\s+INTO\s+{_TABLE_IDENTIFIER}", ]: for match in re.finditer(pattern, sql, re.IGNORECASE): + if not _is_table_name(match.group(1)): + continue t = match.group(1).lower() if t not in seen: seen.add(t) diff --git a/tests/test_adapters/test_sql.py b/tests/test_adapters/test_sql.py index b3a21b6..4c05702 100644 --- a/tests/test_adapters/test_sql.py +++ b/tests/test_adapters/test_sql.py @@ -25,6 +25,24 @@ def test_empty(self): assert extract_tables_from_sql("") == [] assert extract_tables_from_sql(None) == [] + def test_unqualified_names(self): + sql = "INSERT INTO feature_store SELECT * FROM raw_events" + assert extract_tables_from_sql(sql) == ["raw_events"] + + def test_unqualified_join(self): + tables = extract_tables_from_sql("SELECT * FROM orders JOIN customers ON 1=1") + assert tables == ["orders", "customers"] + + def test_mixed_qualified_and_bare(self): + tables = extract_tables_from_sql("SELECT * FROM schema.table1 JOIN raw_events ON 1=1") + assert tables == ["schema.table1", "raw_events"] + + def test_bare_sql_keywords_not_extracted(self): + # Subqueries / table functions must not surface keywords as tables. + assert extract_tables_from_sql("SELECT * FROM LATERAL (SELECT 1)") == [] + assert extract_tables_from_sql("SELECT * FROM TABLE(my_function())") == [] + assert extract_tables_from_sql("SELECT * FROM VALUES (1), (2)") == [] + class TestExtractWriteTables: def test_insert_into(self): @@ -43,6 +61,27 @@ def test_empty(self): assert extract_write_tables("") == [] assert extract_write_tables(None) == [] + def test_unqualified_insert(self): + assert extract_write_tables("INSERT INTO feature_store SELECT * FROM raw_events") == [ + "feature_store" + ] + + def test_unqualified_create(self): + assert extract_write_tables("CREATE TABLE staging AS SELECT 1") == ["staging"] + + def test_unqualified_merge(self): + assert extract_write_tables("MERGE INTO targets USING src ON 1=1") == ["targets"] + + def test_docstring_example_extractable_by_sibling(self): + # The extract_write_tables docstring example must be fully parseable + # by both extractors. + sql = "INSERT INTO schema.output SELECT * FROM source" + assert extract_write_tables(sql) == ["schema.output"] + assert extract_tables_from_sql(sql) == ["source"] + + def test_insert_overwrite_keyword_not_extracted(self): + assert extract_write_tables("INSERT OVERWRITE INTO schema.t SELECT 1") != ["OVERWRITE"] + class TestExtractModelNameFilters: def test_equals(self): From 534914270ef7b16723f820a9e3b7e960b72d2a6c Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:53:44 -0700 Subject: [PATCH 05/22] fix: strip spaced template vars; correct strip_template_vars docstring example The regex only matched {{var}} with no interior whitespace, so standard spaced template variables like {{ ds }} passed through untouched and broke downstream SQL parsing. It now tolerates whitespace inside the braces. The docstring example also claimed an output the function never produced; it now shows the actual result, and a doctest-shaped test pins it. Co-Authored-By: Claude Fable 5 --- src/model_ledger/adapters/sql.py | 12 ++++++++---- tests/test_adapters/test_sql.py | 12 ++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/model_ledger/adapters/sql.py b/src/model_ledger/adapters/sql.py index 6dccd7b..90bf464 100644 --- a/src/model_ledger/adapters/sql.py +++ b/src/model_ledger/adapters/sql.py @@ -191,13 +191,17 @@ def extract_comment_tags(sql: str | None, prefix: str = "@") -> dict[str, str]: def strip_template_vars(sql: str | None) -> str: """Strip {{var}} template wrappers from SQL. - Converts {{schema}}.{{table}} → schema.table. - Common in ETL platforms that use template variables in SQL. + Converts ``{{schema}}.{{table}}`` → ``schema.table``. Whitespace inside + the braces is tolerated, so spaced variables like ``{{ ds }}`` (common + in scheduler-templated SQL) are stripped too. Common in ETL platforms + that use template variables in SQL. Example: >>> strip_template_vars("SELECT * FROM {{schema}}.{{table_name}}") - 'SELECT * FROM app_compliance.cash.table' + 'SELECT * FROM schema.table_name' + >>> strip_template_vars("SELECT * FROM {{ analytics }}.{{ events }}") + 'SELECT * FROM analytics.events' """ if not sql: return "" - return re.sub(r"\{\{(\w+)\}\}", r"\1", sql) + return re.sub(r"\{\{\s*(\w+)\s*\}\}", r"\1", sql) diff --git a/tests/test_adapters/test_sql.py b/tests/test_adapters/test_sql.py index 4c05702..c65b179 100644 --- a/tests/test_adapters/test_sql.py +++ b/tests/test_adapters/test_sql.py @@ -113,6 +113,18 @@ def test_in_query(self): result = strip_template_vars("SELECT * FROM {{app}}.{{cash}}.my_table") assert result == "SELECT * FROM app.cash.my_table" + def test_strips_spaced_vars(self): + result = strip_template_vars( + "SELECT * FROM {{ schema }}.{{ table }} WHERE run_date = '{{ ds }}'" + ) + assert result == "SELECT * FROM schema.table WHERE run_date = 'ds'" + + def test_docstring_example_is_accurate(self): + assert ( + strip_template_vars("SELECT * FROM {{schema}}.{{table_name}}") + == "SELECT * FROM schema.table_name" + ) + def test_no_templates(self): assert strip_template_vars("SELECT 1") == "SELECT 1" From a0e096c8ad87b2fc9b8c07b272b83695660845fa Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:54:14 -0700 Subject: [PATCH 06/22] test: use generic identifiers in DataPort case-normalization test Per the repo's boundary rule, examples in tests use generic names. Co-Authored-By: Claude Fable 5 --- tests/test_graph/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_graph/test_models.py b/tests/test_graph/test_models.py index 09bff31..49b8ad1 100644 --- a/tests/test_graph/test_models.py +++ b/tests/test_graph/test_models.py @@ -10,7 +10,7 @@ def test_create(self): assert p.schema == {} def test_lowercases(self): - assert DataPort("APP_COMPLIANCE.CASH.TABLE").identifier == "app_compliance.cash.table" + assert DataPort("ANALYTICS.EVENTS.TABLE").identifier == "analytics.events.table" def test_equality(self): assert DataPort("table_a") == DataPort("table_a") From f471de8317de13e47f1a4e7906f1594985a6f8d2 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:54:52 -0700 Subject: [PATCH 07/22] test: skip, don't fail, pandas-dependent tests when pandas is absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six tests imported pandas mid-test with no importorskip (4 sample-model factory tests in test_datasets, plus one feature-names test each in test_introspect/test_sklearn and test_lightgbm). With sklearn/xgboost/ lightgbm installed but pandas absent — a combination no extra rules out, since pandas is only pulled in via the snowflake extra — the suite hard-failed instead of skipping. All six now importorskip pandas like every other optional dependency in the suite. Co-Authored-By: Claude Fable 5 --- tests/test_datasets.py | 3 +++ tests/test_introspect/test_lightgbm.py | 3 ++- tests/test_introspect/test_sklearn.py | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test_datasets.py b/tests/test_datasets.py index d3d9233..49d6295 100644 --- a/tests/test_datasets.py +++ b/tests/test_datasets.py @@ -41,6 +41,9 @@ def test_make_rule_engine(): xgboost = pytest.importorskip("xgboost") +# The sample-model factories below build training frames with pandas, which +# no extra guarantees alongside the ML libs — skip, don't fail, without it. +pandas = pytest.importorskip("pandas") def test_make_fraud_detector(): diff --git a/tests/test_introspect/test_lightgbm.py b/tests/test_introspect/test_lightgbm.py index f6b3974..10d5c47 100644 --- a/tests/test_introspect/test_lightgbm.py +++ b/tests/test_introspect/test_lightgbm.py @@ -41,7 +41,8 @@ def test_introspect(introspector, fitted_lgb): def test_introspect_feature_names(introspector): import lightgbm as lgb - import pandas as pd + + pd = pytest.importorskip("pandas") X = pd.DataFrame({"feat_a": [1, 2, 3, 4], "feat_b": [5, 6, 7, 8]}) y = [0, 1, 0, 1] diff --git a/tests/test_introspect/test_sklearn.py b/tests/test_introspect/test_sklearn.py index 7ba7d68..e4728db 100644 --- a/tests/test_introspect/test_sklearn.py +++ b/tests/test_introspect/test_sklearn.py @@ -51,7 +51,7 @@ def test_introspect_logistic_regression(introspector, fitted_lr): def test_introspect_with_feature_names(introspector): - import pandas as pd + pd = pytest.importorskip("pandas") X = pd.DataFrame({"age": [1, 2, 3, 4], "income": [10, 20, 30, 40]}) y = [0, 1, 0, 1] From 8dfb42c47e670dc37271bcc611694bb53b3f117f Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:55:32 -0700 Subject: [PATCH 08/22] chore: remove vestigial excel extra The excel extra declared openpyxl>=3.1, but nothing in src/ or tests/ has imported openpyxl since the scanner module was removed (post-0.7.7). Drop the extra and its row in the installation guide. Co-Authored-By: Claude Fable 5 --- docs/installation.md | 1 - pyproject.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/docs/installation.md b/docs/installation.md index d0ec78f..dd52607 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -27,7 +27,6 @@ uv add model-ledger | `model-ledger[introspect-sklearn]` | scikit-learn introspector | extract algorithm/features from fitted models | | `model-ledger[introspect-xgboost]` | XGBoost introspector | " | | `model-ledger[introspect-lightgbm]` | LightGBM introspector | " | -| `model-ledger[excel]` | openpyxl | spreadsheet import/export | | `model-ledger[all]` | Snowflake + pandas + httpx | the common production set | Combine them: `pip install "model-ledger[mcp,rest-api,snowflake]"`. diff --git a/pyproject.toml b/pyproject.toml index 31cc59a..2970d31 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -43,7 +43,6 @@ dependencies = [ [project.optional-dependencies] cli = ["typer>=0.9", "rich>=13.0"] -excel = ["openpyxl>=3.1"] introspect-sklearn = ["scikit-learn>=1.0"] introspect-xgboost = ["xgboost>=1.7"] introspect-lightgbm = ["lightgbm>=3.0"] From a457d6686e130c13b9bffd2b2172e7ee402e7f25 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:56:54 -0700 Subject: [PATCH 09/22] fix: create_app/create_server reject non-backend objects at construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both factories accepted any object as backend — passing a Ledger (or a bare database path) by mistake failed deep inside the first request with a confusing AttributeError. A shared validate_backend() guard now raises a clear TypeError at construction, with targeted hints for the two common mistakes (a Ledger instance, a path string). Co-Authored-By: Claude Fable 5 --- src/model_ledger/backends/ledger_protocol.py | 23 +++++++++++++++++++ src/model_ledger/mcp/server.py | 4 +++- src/model_ledger/rest/app.py | 7 +++++- tests/test_mcp/test_server.py | 24 ++++++++++++++++++++ tests/test_rest/test_app.py | 20 ++++++++++++++++ 5 files changed, 76 insertions(+), 2 deletions(-) diff --git a/src/model_ledger/backends/ledger_protocol.py b/src/model_ledger/backends/ledger_protocol.py index c147a9f..1de3597 100644 --- a/src/model_ledger/backends/ledger_protocol.py +++ b/src/model_ledger/backends/ledger_protocol.py @@ -8,6 +8,29 @@ from model_ledger.core.ledger_models import ModelRef, Snapshot, Tag +def validate_backend(backend: object | None) -> None: + """Raise ``TypeError`` early if ``backend`` is not a ``LedgerBackend``. + + Catches the common mistake of passing a ``Ledger`` (or a database path) + where a storage backend is expected — which would otherwise fail deep + inside the first operation with a confusing ``AttributeError``. + """ + if backend is None or isinstance(backend, LedgerBackend): + return + from model_ledger.sdk.ledger import Ledger + + hint = "" + if isinstance(backend, Ledger): + hint = ( + " — pass the Ledger's storage backend (e.g. SQLiteLedgerBackend), not the Ledger itself" + ) + elif isinstance(backend, str): + hint = " — construct a backend from the path first (e.g. SQLiteLedgerBackend(path))" + raise TypeError( + f"backend must implement the LedgerBackend protocol; got {type(backend).__name__}{hint}" + ) + + @runtime_checkable class LedgerBackend(Protocol): def save_model(self, model: ModelRef) -> None: ... diff --git a/src/model_ledger/mcp/server.py b/src/model_ledger/mcp/server.py index beb557a..cf87387 100644 --- a/src/model_ledger/mcp/server.py +++ b/src/model_ledger/mcp/server.py @@ -19,7 +19,7 @@ from mcp.server.fastmcp import FastMCP from model_ledger.backends.ledger_memory import InMemoryLedgerBackend -from model_ledger.backends.ledger_protocol import LedgerBackend +from model_ledger.backends.ledger_protocol import LedgerBackend, validate_backend from model_ledger.sdk.ledger import Ledger from model_ledger.tools import schemas from model_ledger.tools.changelog import changelog as _changelog @@ -49,6 +49,8 @@ def create_server( """ from model_ledger.backends.http import HttpLedgerBackend + validate_backend(backend) + # HTTP backend → pass-through mode: call REST API directly if isinstance(backend, HttpLedgerBackend): return _create_http_server(backend) diff --git a/src/model_ledger/rest/app.py b/src/model_ledger/rest/app.py index a301e1f..97981de 100644 --- a/src/model_ledger/rest/app.py +++ b/src/model_ledger/rest/app.py @@ -25,7 +25,7 @@ from model_ledger import __version__ from model_ledger.backends import batch_fallbacks -from model_ledger.backends.ledger_protocol import LedgerBackend +from model_ledger.backends.ledger_protocol import LedgerBackend, validate_backend from model_ledger.core.exceptions import ModelNotFoundError from model_ledger.sdk.ledger import Ledger from model_ledger.tools.changelog import changelog as changelog_fn @@ -66,7 +66,12 @@ def create_app( Returns: A configured FastAPI application. + + Raises: + TypeError: If ``backend`` does not implement the LedgerBackend + protocol (e.g. a ``Ledger`` was passed by mistake). """ + validate_backend(backend) ledger = Ledger(backend=backend) if demo: diff --git a/tests/test_mcp/test_server.py b/tests/test_mcp/test_server.py index 2fc21f3..e16600d 100644 --- a/tests/test_mcp/test_server.py +++ b/tests/test_mcp/test_server.py @@ -203,3 +203,27 @@ def test_main_is_callable(self): from model_ledger.mcp.server import main assert callable(main) + + +class TestCreateServerBackendGuard: + """create_server validates the backend at construction, not first call.""" + + def test_rejects_ledger_instance(self): + from model_ledger.mcp.server import create_server + from model_ledger.sdk.ledger import Ledger + + with pytest.raises(TypeError, match="LedgerBackend"): + create_server(backend=Ledger()) + + def test_rejects_arbitrary_object(self): + from model_ledger.mcp.server import create_server + + with pytest.raises(TypeError, match="LedgerBackend"): + create_server(backend="./ledger.db") + + def test_accepts_real_backend(self): + from model_ledger.backends.ledger_memory import InMemoryLedgerBackend + from model_ledger.mcp.server import create_server + + server = create_server(backend=InMemoryLedgerBackend()) + assert server is not None diff --git a/tests/test_rest/test_app.py b/tests/test_rest/test_app.py index 9d26ecb..b0a5e0a 100644 --- a/tests/test_rest/test_app.py +++ b/tests/test_rest/test_app.py @@ -260,3 +260,23 @@ def test_discover_inline(self, client): assert resp.status_code == 200 data = resp.json() assert data["models_added"] == 2 + + +class TestCreateAppBackendGuard: + """create_app validates the backend at construction, not first request.""" + + def test_rejects_ledger_instance(self): + from model_ledger.sdk.ledger import Ledger + + with pytest.raises(TypeError, match="LedgerBackend"): + create_app(backend=Ledger()) + + def test_rejects_arbitrary_object(self): + with pytest.raises(TypeError, match="LedgerBackend"): + create_app(backend="./ledger.db") + + def test_accepts_real_backend(self): + from model_ledger.backends.ledger_memory import InMemoryLedgerBackend + + app = create_app(backend=InMemoryLedgerBackend()) + assert TestClient(app).get("/overview").status_code == 200 From 166ae7049b2e6a175227d0bf626b02df1974fbbc Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 16:58:21 -0700 Subject: [PATCH 10/22] fix: sql_connector raises a clear error for tuple-row connections sql_connector addresses row values by column name, but a connection returning raw DB-API tuple rows (e.g. default sqlite3) crashed with an opaque 'TypeError: tuple indices must be integers or slices, not str' deep inside row mapping. discover() now checks each row is a mapping and raises a TypeError that names the requirement and the fix (dict row_factory / DictCursor). Docs: the connectors guide and the factory docstring now state the dict-row requirement with a sqlite3 row_factory example. Co-Authored-By: Claude Fable 5 --- docs/guides/connectors.md | 15 +++++++ src/model_ledger/connectors/sql.py | 17 +++++++- tests/test_connectors/test_sql_connector.py | 48 +++++++++++++++++++++ 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/docs/guides/connectors.md b/docs/guides/connectors.md index cc3cb40..562311a 100644 --- a/docs/guides/connectors.md +++ b/docs/guides/connectors.md @@ -38,6 +38,21 @@ ledger.add(etl_jobs.discover()) ledger.connect() # links ETL outputs to model inputs automatically ``` +!!! note "The connection must return dict-style rows" + `sql_connector` addresses row values by column name (`row["owner"]`), so the + connection's `execute()` must return mappings, not raw DB-API tuples — tuple + rows raise a `TypeError` at discovery. For `sqlite3`, set a dict row factory; + for most other drivers, use the `DictCursor` equivalent: + + ```python + import sqlite3 + + conn = sqlite3.connect("registry.db") + conn.row_factory = lambda cursor, row: { + d[0]: row[i] for i, d in enumerate(cursor.description) + } + ``` + ## REST APIs Works with MLflow, SageMaker, Vertex AI, or any JSON API: diff --git a/src/model_ledger/connectors/sql.py b/src/model_ledger/connectors/sql.py index bd00f86..06c4819 100644 --- a/src/model_ledger/connectors/sql.py +++ b/src/model_ledger/connectors/sql.py @@ -35,7 +35,10 @@ def sql_connector( Args: name: Platform name for discovered DataNodes. - connection: Database connection with execute() method. + connection: Database connection with an execute() method that returns + dict-style rows (mappings addressable by column name). Raw DB-API + tuple rows are rejected — configure a dict row factory (sqlite3) + or a DictCursor (most drivers) first. query: SQL query to run. name_column: Column containing the model name. name_prefix: Optional prefix for model names (e.g., "queue:"). @@ -145,7 +148,17 @@ def __init__( def discover(self) -> list[DataNode]: rows = self._conn.execute(self._query) - return [self._to_node(row) for row in rows] + nodes: list[DataNode] = [] + for row in rows: + if not hasattr(row, "get"): + raise TypeError( + "sql_connector requires dict-style rows (mappings addressable " + f"by column name), got {type(row).__name__}. Configure the " + "connection to return mappings — e.g. set a dict row_factory " + "on sqlite3, or use your driver's DictCursor." + ) + nodes.append(self._to_node(row)) + return nodes def _build_port(self, row: dict[str, Any], port_cfg: dict[str, str]) -> DataPort | None: """Build a DataPort from a port config dict.""" diff --git a/tests/test_connectors/test_sql_connector.py b/tests/test_connectors/test_sql_connector.py index 5c12827..fc4ebf1 100644 --- a/tests/test_connectors/test_sql_connector.py +++ b/tests/test_connectors/test_sql_connector.py @@ -334,3 +334,51 @@ def test_integrates_with_ledger(): ledger.add(reader_conn.discover()) result = ledger.connect() assert result["links_created"] >= 1 + + +class TestDictRowRequirement: + """Connections returning tuple rows must fail with a clear error.""" + + def test_tuple_rows_raise_clear_type_error(self, tmp_path): + import sqlite3 + + import pytest + + db = tmp_path / "registry.db" + conn = sqlite3.connect(db) # default row factory: tuples + conn.execute("CREATE TABLE models (name TEXT, owner TEXT)") + conn.execute("INSERT INTO models VALUES ('model_a', 'alice')") + conn.commit() + + c = sql_connector( + name="registry", + connection=conn, + query="SELECT name, owner FROM models", + name_column="name", + ) + with pytest.raises(TypeError, match="dict-style rows"): + c.discover() + conn.close() + + def test_dict_row_factory_works(self, tmp_path): + import sqlite3 + + db = tmp_path / "registry.db" + conn = sqlite3.connect(db) + conn.execute("CREATE TABLE models (name TEXT, owner TEXT)") + conn.execute("INSERT INTO models VALUES ('model_a', 'alice')") + conn.commit() + conn.row_factory = lambda cursor, row: { + d[0]: row[i] for i, d in enumerate(cursor.description) + } + + c = sql_connector( + name="registry", + connection=conn, + query="SELECT name, owner FROM models", + name_column="name", + ) + nodes = c.discover() + assert nodes[0].name == "model_a" + assert nodes[0].metadata["owner"] == "alice" + conn.close() From cc55c41402b31f93dc607fe672d862dd6e4365b3 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:00:00 -0700 Subject: [PATCH 11/22] fix(cli): export help said directory; it writes a single file model-ledger export --output described an 'Output directory' and the success message read 'exported to pack_out/', but the command writes one HTML file at exactly the given path (previously extensionless by default). Help now says 'Output file path', the default gained a proper .html extension, and the success message prints the real artifact path. A richer artifact layout (per-format extensions or a real directory) is a deliberate CLI change deferred to a later release. CLI test asserts the help text and success message match the artifact actually produced. Co-Authored-By: Claude Fable 5 --- src/model_ledger/cli/app.py | 8 +++++--- tests/test_cli/test_app.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/model_ledger/cli/app.py b/src/model_ledger/cli/app.py index 5e67480..164d44f 100644 --- a/src/model_ledger/cli/app.py +++ b/src/model_ledger/cli/app.py @@ -373,7 +373,9 @@ def export_cmd( model_name: str = typer.Argument(help="Name of the model to export."), db: str = typer.Option(default=None, help="Path to the inventory database."), version: str | None = typer.Option(None, help="Version to export. Defaults to latest."), - output: str = typer.Option("audit_pack", help="Output directory for the audit pack."), + output: str = typer.Option( + "audit_pack.html", help="Output file path for the audit pack (a single HTML file)." + ), ) -> None: """Export an audit pack for a model version.""" db = db or _default_db() @@ -397,11 +399,11 @@ def export_cmd( from model_ledger.export.audit_pack import export_audit_pack export_audit_pack(inventory=inv, model_name=model_name, version=version, output_path=output) - console.print(f"[green]Audit pack exported to {output}/[/green]") + console.print(f"[green]Audit pack exported to {output}[/green]") except (ImportError, AttributeError): console.print( f"[yellow]Export not yet implemented.[/yellow] " - f"Would export audit pack for {model_name} v{version} to {output}/" + f"Would export audit pack for {model_name} v{version} to {output}" ) diff --git a/tests/test_cli/test_app.py b/tests/test_cli/test_app.py index 7ad5454..d0591b0 100644 --- a/tests/test_cli/test_app.py +++ b/tests/test_cli/test_app.py @@ -113,3 +113,39 @@ def test_validate_unknown_profile_exits_cleanly(tmp_path): result = runner.invoke(app, ["validate", "test-model", "--db", db, "--profile", "nope"]) assert result.exit_code == 1 assert "Unknown profile" in result.output + + +def _inventory_with_version(db): + inv = Inventory(db_path=db) + inv.register_model(name="test-model", owner="tester", tier="low", intended_purpose="testing") + with inv.new_version("test-model") as v: + v.add_component("Processing/algorithm", type="algorithm") + return inv + + +def test_export_help_describes_a_file_path(): + result = runner.invoke(app, ["export", "--help"]) + assert result.exit_code == 0 + assert "file path" in result.output.lower() + assert "directory" not in result.output.lower() + + +def test_export_message_matches_produced_artifact(tmp_path): + db = str(tmp_path / "test.db") + _inventory_with_version(db) + out = str(tmp_path / "pack_out.html") + + result = runner.invoke(app, ["export", "test-model", "--db", db, "--output", out]) + + assert result.exit_code == 0 + # A single file is produced at the given path... + from pathlib import Path + + artifact = Path(out) + assert artifact.is_file() + assert not artifact.is_dir() + # ...and the success message names that path, not a directory. + # (Rich wraps long lines, so unwrap before matching.) + output = result.output.replace("\n", "") + assert f"exported to {out}" in output + assert f"{out}/" not in output From efa2fd59f6c3579c4ca06ac433e29de9da145783 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:01:13 -0700 Subject: [PATCH 12/22] docs: backfill CHANGELOG for 0.7.10-0.7.12; add 0.7.13 entry; bump version CHANGELOG.md stopped at v0.7.9 while the changelog URL is advertised in pyproject and the README. Backfills v0.7.10 (#32, #33), v0.7.11 (#34), and v0.7.12 (#35) from the release history, adds the v0.7.13 section for this fix batch, and bumps the package version to 0.7.13. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 27 +++++++++++++++++++++++++++ pyproject.toml | 2 +- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 503ba0e..40101d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,32 @@ # Changelog +## v0.7.13 (Unreleased) + +- fix: `Ledger(HttpLedgerBackend(url))` no longer corrupts the remote ledger. `append_snapshot` sends the real model name instead of `""` (which auto-registered a phantom empty-name model and attached every recorded event to it) and raises `ModelNotFoundError` for unresolvable hashes; registration logs exactly one `registered` event instead of double-firing (re-registration is now idempotent); `get_snapshot()` serves appended snapshots from a client-side cache and reconstructs older ones from `GET /changelog` instead of always returning `None`. Defense in depth: `RecordInput.model_name` requires `min_length=1`, so old clients that POST an empty model name get a 422 instead of silently corrupting the inventory. A new integration suite drives the SDK against the real `create_app()` handlers (not mocks). +- fix: the `query` tool's `platform` filter is actually applied. It was advertised in `QueryInput` and the REST/MCP schemas but never read — any platform value returned the full inventory. Platform is resolved from snapshot data in one batched `batch_platforms` dispatch and filtered/paginated in the tool; a schema-vs-implementation parity test now guards every `QueryInput` filter field. +- fix: the introspector registry dispatches XGBoost sklearn-API models to the xgboost introspector. `SklearnIntrospector.can_handle` now rejects estimators from wrapper frameworks with dedicated introspectors (xgboost, lightgbm) instead of claiming them via `BaseEstimator`, making dispatch independent of entry-point ordering. +- fix: SQL lineage extraction accepts unqualified (bare) table names — `INSERT INTO feature_store SELECT * FROM raw_events` previously parsed to zero reads and zero writes, silently producing no lineage. A keyword blocklist keeps subqueries, table functions, and row sources out of the results; `INSERT OVERWRITE` is recognized. +- fix: `strip_template_vars` strips spaced template variables like `{{ ds }}`; its docstring example now shows the actual output. +- fix: `sql_connector` raises a clear `TypeError` naming the dict-row requirement when the connection yields tuple rows, instead of an opaque indexing error. The connectors guide documents the requirement with a sqlite3 `row_factory` example. +- fix: `create_app()` / `create_server()` validate the backend at construction and raise a clear `TypeError` (with hints for the common Ledger-instead-of-backend and path-string mistakes) instead of failing deep inside the first request with `AttributeError`. +- fix(cli): `export --output` help text now says "Output file path" (the command writes a single file, not a directory), the default output gained a proper `.html` extension, and the success message prints the real artifact path. +- feat: `Ledger.register()` accepts an optional `payload` dict merged into the registration event's payload (caller keys win) — used by the record tool so the single `registered` event still carries the caller's payload. +- chore: remove the vestigial `excel` extra — nothing has imported openpyxl since the scanner module was removed post-v0.7.7. +- test: pandas-dependent tests skip instead of hard-failing when pandas is absent alongside the ML introspection libs. + +## v0.7.12 + +- fix: escape backslashes in single-quoted SQL literals — JSON payloads containing escaped double quotes (`\"`) or backslashes reached `PARSE_JSON` mangled on the SQL fallback write path, which re-introduced the poisoned-buffer failure (INSERT fails, buffer never clears, every subsequent flush-before-read 500s) on common data. Also from the post-merge review of #33/#34: privilege denials short-circuit quietly again instead of warning per flush, unexpected pandas-path failures log with `exc_info`, and the failure-path staging-table DROP is gone. (#35) + +## v0.7.11 + +- fix: the pandas bulk write path falls back to the DDL-free SQL path on ANY failure, not just privilege errors. Observed in production: `write_pandas` failing inside the file-transfer agent (not a privilege error) propagated, left the snapshot buffer poisoned, and 500'd every subsequent read on the process. Any pandas-path failure now logs a warning, best-effort drops the staging table, clears cleanly, and falls back. (#34) + +## v0.7.10 + +- fix: the SQL fallback snapshot flush carries PAYLOAD and TAGS — the DDL-free path taken by least-privilege deployments previously inserted only the 7 scalar columns, silently persisting every snapshot with NULL payload and tags. (#33) +- ci: publish to the official MCP Registry via GitHub OIDC on each release. (#32) + ## v0.7.9 - chore: publish to the official MCP Registry as `io.github.block/model-ledger` — adds `server.json` and the PyPI ownership marker in the README (#31) diff --git a/pyproject.toml b/pyproject.toml index 2970d31..9c49cb7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "model-ledger" -version = "0.7.12" +version = "0.7.13" description = "Developer-first model inventory and governance framework for SR 11-7, EU AI Act, and NIST AI RMF compliance" readme = "README.md" requires-python = ">=3.10" From a04267700500e3c53ec969e69466bf5ce37b1c62 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:12:19 -0700 Subject: [PATCH 13/22] fix(trace): depth is the real BFS level, not flat-list position The trace tool computed each node's depth from its index in the flat transitive dependency list (depth = total - idx), so any model with N transitive upstreams rendered as an N-deep linear chain regardless of the graph's actual shape. Downstream traversal had the same fabrication. Traversal is now an explicit breadth-first walk that records each node's actual level (shortest edge distance from the traced model, one entry per node at its minimum depth) and terminates at input.depth, so depth-bounded queries do depth-bounded work instead of filtering after a full traversal. Regression fixture: 19-node fan-out DAG over 8 uneven levels with a diamond and a shortcut edge; 6 new tests fail on the old implementation. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + src/model_ledger/tools/trace.py | 95 ++++++++++++--------- tests/test_tools/test_trace.py | 147 ++++++++++++++++++++++++++++++++ 3 files changed, 202 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40101d8..6d5706f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - fix: the `query` tool's `platform` filter is actually applied. It was advertised in `QueryInput` and the REST/MCP schemas but never read — any platform value returned the full inventory. Platform is resolved from snapshot data in one batched `batch_platforms` dispatch and filtered/paginated in the tool; a schema-vs-implementation parity test now guards every `QueryInput` filter field. - fix: the introspector registry dispatches XGBoost sklearn-API models to the xgboost introspector. `SklearnIntrospector.can_handle` now rejects estimators from wrapper frameworks with dedicated introspectors (xgboost, lightgbm) instead of claiming them via `BaseEstimator`, making dispatch independent of entry-point ordering. - fix: SQL lineage extraction accepts unqualified (bare) table names — `INSERT INTO feature_store SELECT * FROM raw_events` previously parsed to zero reads and zero writes, silently producing no lineage. A keyword blocklist keeps subqueries, table functions, and row sources out of the results; `INSERT OVERWRITE` is recognized. +- fix: the `trace` tool reports real BFS depth instead of fabricating it from list position. `depth` was computed from each node's index in the flat transitive list, so any model with N transitive upstreams rendered as an N-deep linear chain (a rich 8-level fan-out graph showed as "43 nodes at depths 1–43"); downstream had the same bug. Traversal is now a breadth-first walk that records each node's actual level (shortest edge distance, one entry per node at its minimum depth) and terminates at `input.depth` — bounded queries now do bounded work instead of filtering after a full traversal. - fix: `strip_template_vars` strips spaced template variables like `{{ ds }}`; its docstring example now shows the actual output. - fix: `sql_connector` raises a clear `TypeError` naming the dict-row requirement when the connection yields tuple rows, instead of an opaque indexing error. The connectors guide documents the requirement with a sqlite3 `row_factory` example. - fix: `create_app()` / `create_server()` validate the backend at construction and raise a clear `TypeError` (with hints for the common Ledger-instead-of-backend and path-string mistakes) instead of failing deep inside the first request with `AttributeError`. diff --git a/src/model_ledger/tools/trace.py b/src/model_ledger/tools/trace.py index 74a378a..ebf0bde 100644 --- a/src/model_ledger/tools/trace.py +++ b/src/model_ledger/tools/trace.py @@ -5,16 +5,55 @@ import contextlib from model_ledger.backends import batch_fallbacks +from model_ledger.core.exceptions import ModelNotFoundError from model_ledger.sdk.ledger import Ledger from model_ledger.tools.schemas import DependencyNode, TraceInput, TraceOutput +def _bfs_levels( + ledger: Ledger, + root: str, + direction: str, + max_depth: int | None, +) -> list[tuple[str, int]]: + """Breadth-first traversal from ``root``, recording each node's real level. + + ``depth`` is the BFS level — the shortest edge distance from the traced + node — not a position in a flat transitive list. Each node is reported + once, at its minimum depth. The traversal terminates at ``max_depth``, + so depth-bounded queries do depth-bounded work. + """ + visited: set[str] = {root} + frontier: list[str] = [root] + levels: list[tuple[str, int]] = [] + depth = 0 + while frontier and (max_depth is None or depth < max_depth): + depth += 1 + next_frontier: list[str] = [] + for name in frontier: + try: + edges = ledger.dependencies(name, direction=direction) + except (KeyError, ValueError, ModelNotFoundError): + continue + for edge in edges: + child = edge["model"].name + if child in visited: + continue + visited.add(child) + levels.append((child, depth)) + next_frontier.append(child) + frontier = next_frontier + return levels + + def trace(input: TraceInput, ledger: Ledger) -> TraceOutput: """Traverse a model's dependency graph. Walks upstream (models this one depends on) and/or downstream - (models that depend on this one), returning ``DependencyNode`` lists - with depth and relationship metadata. + (models that depend on this one) breadth-first, returning + ``DependencyNode`` lists whose ``depth`` is the actual BFS level from + the traced model. ``input.depth`` bounds the traversal itself, not + just the output. Raises: ModelNotFoundError: If the target model does not exist. @@ -22,23 +61,16 @@ def trace(input: TraceInput, ledger: Ledger) -> TraceOutput: ledger.get(input.name) backend = ledger._backend - upstream_names: list[str] = [] + upstream_levels: list[tuple[str, int]] = [] if input.direction in ("upstream", "both"): - try: - upstream_names = ledger.upstream(input.name) - except (KeyError, ValueError): - upstream_names = [] + upstream_levels = _bfs_levels(ledger, input.name, "upstream", input.depth) - downstream_names: list[str] = [] + downstream_levels: list[tuple[str, int]] = [] if input.direction in ("downstream", "both"): - try: - downstream_names = ledger.downstream(input.name) - except (KeyError, ValueError): - downstream_names = [] + downstream_levels = _bfs_levels(ledger, input.name, "downstream", input.depth) - all_names = upstream_names + downstream_names name_to_hash: dict[str, str] = {} - for n in all_names: + for n, _ in upstream_levels + downstream_levels: with contextlib.suppress(Exception): name_to_hash[n] = ledger.get(n).model_hash @@ -51,36 +83,17 @@ def trace(input: TraceInput, ledger: Ledger) -> TraceOutput: else: platforms = {} - upstream_nodes: list[DependencyNode] = [] - total_up = len(upstream_names) - for idx, name in enumerate(upstream_names): - mh = name_to_hash.get(name) - platform = platforms.get(mh) if mh else None - upstream_nodes.append( - DependencyNode( - name=name, - platform=platform, - depth=total_up - idx, - relationship="depends_on", - ) - ) - - downstream_nodes: list[DependencyNode] = [] - for idx, name in enumerate(downstream_names): + def _node(name: str, depth: int, relationship: str) -> DependencyNode: mh = name_to_hash.get(name) - platform = platforms.get(mh) if mh else None - downstream_nodes.append( - DependencyNode( - name=name, - platform=platform, - depth=idx + 1, - relationship="feeds_into", - ) + return DependencyNode( + name=name, + platform=platforms.get(mh) if mh else None, + depth=depth, + relationship=relationship, ) - if input.depth is not None: - upstream_nodes = [n for n in upstream_nodes if n.depth <= input.depth] - downstream_nodes = [n for n in downstream_nodes if n.depth <= input.depth] + upstream_nodes = [_node(n, d, "depends_on") for n, d in upstream_levels] + downstream_nodes = [_node(n, d, "feeds_into") for n, d in downstream_levels] total = len(upstream_nodes) + len(downstream_nodes) return TraceOutput( diff --git a/tests/test_tools/test_trace.py b/tests/test_tools/test_trace.py index c6bbacc..e2342f3 100644 --- a/tests/test_tools/test_trace.py +++ b/tests/test_tools/test_trace.py @@ -232,3 +232,150 @@ class TestTraceNonexistentModel: def test_raises_model_not_found(self, ledger): with pytest.raises(ModelNotFoundError): trace(TraceInput(name="nonexistent_model"), ledger) + + +@pytest.fixture +def fanout_ledger(ledger): + """Synthetic fan-out DAG: 19 transitive upstreams of scoring_model + spread over 8 real BFS levels with uneven fan-out, a diamond + (r1 is reachable via both t1 and t2), and a shortcut (r2 is both a + direct upstream and reachable again at level 3). + + Level sizes from scoring_model: [4, 3, 2, 4, 2, 2, 1, 1]. + """ + edges = [ + # (upstream, downstream) — downstream depends_on upstream + ("f1", "scoring_model"), + ("f2", "scoring_model"), + ("f3", "scoring_model"), + ("r2", "scoring_model"), # shortcut: also reachable at level 3 + ("t1", "f1"), + ("t2", "f1"), + ("t3", "f2"), + ("r1", "t1"), + ("r1", "t2"), # diamond: r1 via two paths + ("r2", "t2"), + ("r3", "t3"), + ("s1", "r1"), + ("s2", "r1"), + ("s3", "r1"), + ("s4", "r1"), + ("u1", "s1"), + ("u2", "s4"), + ("v1", "u1"), + ("v2", "u1"), + ("w1", "v1"), + ("x1", "w1"), + ] + names = {"scoring_model"} | {n for e in edges for n in e} + for name in sorted(names): + ledger.register( + name=name, + owner="risk-team", + model_type="ml_model", + tier="low", + purpose="fixture", + ) + for up, down in edges: + ledger.link_dependency(up, down, actor="graph_builder") + return ledger + + +EXPECTED_UPSTREAM_DEPTHS = { + "f1": 1, + "f2": 1, + "f3": 1, + "r2": 1, # shortest path wins over the level-3 route + "t1": 2, + "t2": 2, + "t3": 2, + "r1": 3, + "r3": 3, + "s1": 4, + "s2": 4, + "s3": 4, + "s4": 4, + "u1": 5, + "u2": 5, + "v1": 6, + "v2": 6, + "w1": 7, + "x1": 8, +} + + +class TestTraceDepthIsBfsLevel: + """Regression: depth must be the actual BFS level from the traced node, + not the node's position in a flat transitive list. Previously any model + with N transitive upstreams rendered as an N-deep linear chain.""" + + def test_upstream_depths_are_bfs_levels(self, fanout_ledger): + result = trace(TraceInput(name="scoring_model", direction="upstream"), fanout_ledger) + + depths = {n.name: n.depth for n in result.upstream} + assert depths == EXPECTED_UPSTREAM_DEPTHS + + def test_max_depth_is_level_count_not_node_count(self, fanout_ledger): + result = trace(TraceInput(name="scoring_model", direction="upstream"), fanout_ledger) + + assert len(result.upstream) == 19 + assert max(n.depth for n in result.upstream) == 8 # not 19 + + def test_diamond_node_appears_once_at_min_depth(self, fanout_ledger): + result = trace(TraceInput(name="scoring_model", direction="upstream"), fanout_ledger) + + r1 = [n for n in result.upstream if n.name == "r1"] + assert len(r1) == 1 + assert r1[0].depth == 3 + # Shortcut node: direct edge beats the longer route. + r2 = [n for n in result.upstream if n.name == "r2"] + assert len(r2) == 1 + assert r2[0].depth == 1 + + def test_depth_bound_returns_exactly_the_near_levels(self, fanout_ledger): + result = trace( + TraceInput(name="scoring_model", direction="upstream", depth=3), + fanout_ledger, + ) + + expected = {n for n, d in EXPECTED_UPSTREAM_DEPTHS.items() if d <= 3} + assert {n.name for n in result.upstream} == expected + + def test_downstream_depths_are_bfs_levels(self, fanout_ledger): + """Symmetry: downstream had the same flat-list fabrication.""" + result = trace(TraceInput(name="x1", direction="downstream"), fanout_ledger) + + depths = {n.name: n.depth for n in result.downstream} + assert depths == { + "w1": 1, + "v1": 2, + "u1": 3, + "s1": 4, + "r1": 5, + "t1": 6, + "t2": 6, # fan-out: two nodes share level 6 + "f1": 7, + "scoring_model": 8, + } + + def test_depth_bound_limits_traversal_work(self, fanout_ledger): + """depth must bound the traversal itself, not just filter output.""" + backend = fanout_ledger._backend + calls = {"n": 0} + original = backend.list_snapshots + + def counting_list_snapshots(*args, **kwargs): + calls["n"] += 1 + return original(*args, **kwargs) + + backend.list_snapshots = counting_list_snapshots + try: + trace(TraceInput(name="scoring_model", direction="upstream", depth=1), fanout_ledger) + bounded = calls["n"] + calls["n"] = 0 + trace(TraceInput(name="scoring_model", direction="upstream"), fanout_ledger) + unbounded = calls["n"] + finally: + backend.list_snapshots = original + + assert bounded < unbounded From 02fbe6bdbac3e743099286ae50f33cdb07c73561 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:38:25 -0700 Subject: [PATCH 14/22] fix(sql): CTE names and DELETE FROM targets are not read tables; handle INSERT OVERWRITE TABLE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bare-identifier acceptance turned every WITH-clause binding into a phantom read table — dbt-style multi-CTE SQL reported its own CTE names as lineage inputs, and a CTE sharing a real table's name created spurious cross-model dependency edges. WITH-clause names (including nested and RECURSIVE forms, with optional column lists) are now parsed and subtracted from extracted reads; DELETE FROM targets are likewise excluded (a delete target is not a read). Write extraction now recognizes the Hive/Spark form INSERT OVERWRITE TABLE t, which the keyword filter previously dropped. Docstrings document the keyword-blocklist tradeoff: a bare table genuinely named a filtered keyword must be schema-qualified to extract. Co-Authored-By: Claude Fable 5 --- src/model_ledger/adapters/sql.py | 47 +++++++++++++++- tests/test_adapters/test_sql.py | 94 ++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 3 deletions(-) diff --git a/src/model_ledger/adapters/sql.py b/src/model_ledger/adapters/sql.py index 90bf464..5463d36 100644 --- a/src/model_ledger/adapters/sql.py +++ b/src/model_ledger/adapters/sql.py @@ -33,6 +33,31 @@ _TABLE_IDENTIFIER = r"([a-zA-Z_][a-zA-Z0-9_]*(?:\.[a-zA-Z_][a-zA-Z0-9_]*){0,2})" +# A CTE binding: `WITH [RECURSIVE] name [(cols)] AS (` or a comma-continued +# `, name [(cols)] AS (`. The comma form is only meaningful inside a WITH +# clause, so _extract_cte_names guards on the statement containing WITH. +_CTE_BINDING_PATTERN = re.compile( + r"(?:\bWITH\s+(?:RECURSIVE\s+)?|,\s*)" + r"([a-zA-Z_][a-zA-Z0-9_]*)\s*(?:\([^()]*\)\s*)?AS\s*\(", + re.IGNORECASE, +) + +# `DELETE FROM target` — the target is neither a read nor (for lineage +# purposes here) a write; strip the FROM so the read extractor skips it. +_DELETE_FROM_PATTERN = re.compile(r"\bDELETE\s+FROM\b", re.IGNORECASE) + + +def _extract_cte_names(sql: str) -> set[str]: + """Lowercased WITH-clause binding names (including nested/recursive CTEs). + + CTE names are query-local aliases: a reference to one resolves to the + CTE for the whole statement (even when it shadows a real table), so + they must never surface as read tables. + """ + if not re.search(r"\bWITH\b", sql, re.IGNORECASE): + return set() + return {m.group(1).lower() for m in _CTE_BINDING_PATTERN.finditer(sql)} + def _is_table_name(identifier: str) -> bool: """True unless a bare identifier is really a SQL keyword.""" @@ -44,8 +69,15 @@ def extract_tables_from_sql(sql: str | None) -> list[str]: Handles fully-qualified (``db.schema.table``), schema-qualified (``schema.table``), and bare (``table``) identifiers. + CTE names (``WITH name AS (...)``) and ``DELETE FROM`` targets are + excluded — neither is a read table. Returns deduplicated list preserving first-seen order. + Note: + A bare table genuinely named one of the filtered SQL keywords + (``values``, ``table``, ``dual``, ...) is not extracted; qualify it + (``schema.values``) to force extraction. + Example: >>> extract_tables_from_sql("SELECT * FROM schema.table1 JOIN schema.table2 ON 1=1") ['schema.table1', 'schema.table2'] @@ -54,6 +86,8 @@ def extract_tables_from_sql(sql: str | None) -> list[str]: """ if not sql: return [] + cte_names = _extract_cte_names(sql) + sql = _DELETE_FROM_PATTERN.sub("DELETE", sql) pattern = rf"(?:FROM|JOIN)\s+{_TABLE_IDENTIFIER}" matches = re.findall(pattern, sql, re.IGNORECASE) seen: set[str] = set() @@ -62,6 +96,8 @@ def extract_tables_from_sql(sql: str | None) -> list[str]: if not _is_table_name(table): continue lower = table.lower() + if lower in cte_names: + continue if lower not in seen: seen.add(lower) result.append(table) @@ -71,8 +107,13 @@ def extract_tables_from_sql(sql: str | None) -> list[str]: def extract_write_tables(sql: str | None) -> list[str]: """Extract tables that a SQL statement writes to. - Handles INSERT INTO, CREATE [OR REPLACE] TABLE, MERGE INTO, with - qualified or bare table identifiers. + Handles INSERT INTO, INSERT OVERWRITE [TABLE|INTO], CREATE [OR REPLACE] + TABLE, MERGE INTO, with qualified or bare table identifiers. + + Note: + A bare table genuinely named one of the filtered SQL keywords + (``values``, ``table``, ``dual``, ...) is not extracted; qualify it + (``schema.values``) to force extraction. Example: >>> extract_write_tables("INSERT INTO schema.output SELECT * FROM source") @@ -86,7 +127,7 @@ def extract_write_tables(sql: str | None) -> list[str]: seen: set[str] = set() for pattern in [ - rf"INSERT\s+(?:OVERWRITE\s+)?(?:INTO\s+)?{_TABLE_IDENTIFIER}", + rf"INSERT\s+(?:OVERWRITE\s+)?(?:TABLE\s+|INTO\s+)?{_TABLE_IDENTIFIER}", rf"CREATE\s+(?:OR\s+REPLACE\s+)?TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?{_TABLE_IDENTIFIER}", rf"MERGE\s+INTO\s+{_TABLE_IDENTIFIER}", ]: diff --git a/tests/test_adapters/test_sql.py b/tests/test_adapters/test_sql.py index c65b179..b3dc4a5 100644 --- a/tests/test_adapters/test_sql.py +++ b/tests/test_adapters/test_sql.py @@ -44,6 +44,88 @@ def test_bare_sql_keywords_not_extracted(self): assert extract_tables_from_sql("SELECT * FROM VALUES (1), (2)") == [] +class TestCteNamesExcluded: + """CTE names are query-local aliases, never read tables. + + Bare-identifier acceptance must not turn WITH-clause bindings into + phantom lineage inputs. + """ + + def test_single_cte(self): + sql = ( + "WITH recent_events AS (SELECT * FROM warehouse.raw_events) SELECT * FROM recent_events" + ) + assert extract_tables_from_sql(sql) == ["warehouse.raw_events"] + + def test_multi_cte_dbt_style(self): + sql = """ + WITH base AS ( + SELECT * FROM raw.events + ), enriched AS ( + SELECT * FROM base JOIN raw.users ON base.user_id = raw.users.id + ), final AS ( + SELECT * FROM enriched + ) + SELECT * FROM final + """ + assert extract_tables_from_sql(sql) == ["raw.events", "raw.users"] + + def test_nested_with(self): + sql = ( + "WITH outer_cte AS (" + " WITH inner_cte AS (SELECT * FROM warehouse.raw_events)" + " SELECT * FROM inner_cte" + ") SELECT * FROM outer_cte JOIN warehouse.users ON 1=1" + ) + assert extract_tables_from_sql(sql) == ["warehouse.raw_events", "warehouse.users"] + + def test_with_recursive(self): + sql = ( + "WITH RECURSIVE ancestry AS (" + " SELECT * FROM org_chart" + " UNION ALL" + " SELECT * FROM ancestry JOIN org_chart ON 1=1" + ") SELECT * FROM ancestry" + ) + assert extract_tables_from_sql(sql) == ["org_chart"] + + def test_cte_with_column_list(self): + sql = "WITH cte (a, b) AS (SELECT 1, 2 FROM real_table) SELECT * FROM cte" + assert extract_tables_from_sql(sql) == ["real_table"] + + def test_cte_shadowing_real_table_is_excluded(self): + # A CTE that reuses a real table's name shadows it for the whole + # statement; references resolve to the CTE, so nothing is extracted. + sql = "WITH raw_events AS (SELECT * FROM warehouse.archive) SELECT * FROM raw_events" + assert extract_tables_from_sql(sql) == ["warehouse.archive"] + + def test_cte_feeding_a_write_statement(self): + sql = "WITH cte AS (SELECT 1) INSERT INTO out_table SELECT * FROM cte" + assert extract_tables_from_sql(sql) == [] + assert extract_write_tables(sql) == ["out_table"] + + def test_original_bare_from_still_resolves(self): + # The OSS-4 headline case must keep working with the CTE filter on. + sql = "INSERT INTO feature_store SELECT * FROM raw_events" + assert extract_tables_from_sql(sql) == ["raw_events"] + + def test_case_insensitive_exclusion(self): + sql = "WITH Recent AS (SELECT * FROM warehouse.raw_events) SELECT * FROM RECENT" + assert extract_tables_from_sql(sql) == ["warehouse.raw_events"] + + +class TestDeleteFromNotARead: + def test_bare_delete_target_not_extracted(self): + assert extract_tables_from_sql("DELETE FROM audit_log WHERE 1=1") == [] + + def test_qualified_delete_target_not_extracted(self): + assert extract_tables_from_sql("DELETE FROM schema.audit_log WHERE 1=1") == [] + + def test_delete_with_using_keeps_the_read_source(self): + sql = "DELETE FROM audit_log USING retention_policy WHERE 1=1" + assert "audit_log" not in extract_tables_from_sql(sql) + + class TestExtractWriteTables: def test_insert_into(self): assert "schema.output" in extract_write_tables("INSERT INTO schema.output SELECT 1") @@ -82,6 +164,18 @@ def test_docstring_example_extractable_by_sibling(self): def test_insert_overwrite_keyword_not_extracted(self): assert extract_write_tables("INSERT OVERWRITE INTO schema.t SELECT 1") != ["OVERWRITE"] + def test_insert_overwrite_table_hive_form(self): + # Hive/Spark: INSERT OVERWRITE TABLE ... + sql = "INSERT OVERWRITE TABLE feature_out SELECT * FROM src" + assert extract_write_tables(sql) == ["feature_out"] + + def test_insert_overwrite_table_qualified(self): + sql = "INSERT OVERWRITE TABLE warehouse.feature_out SELECT * FROM src" + assert extract_write_tables(sql) == ["warehouse.feature_out"] + + def test_insert_overwrite_bare(self): + assert extract_write_tables("INSERT OVERWRITE feature_out SELECT 1") == ["feature_out"] + class TestExtractModelNameFilters: def test_equals(self): From 2b53755881894f263ecd5618c4523bc5232618e5 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:41:25 -0700 Subject: [PATCH 15/22] fix(http): list_snapshots fetches the full history, not the changelog's 7-day default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changelog tool defaults since = now - 7 days when no bounds are given, and HttpLedgerBackend.list_snapshots called GET /changelog without one — so everything built on it was silently wrong for any model whose events are older than a week: get_snapshot reconstruction returned None, latest_snapshot returned None (Ledger.tag raised ModelNotFoundError on a healthy month-old model), the record tool's idempotency check missed the registration event, and the platform query filter returned total=0 for a known platform. list_snapshots now passes an explicit all-time since bound. Also cache the name mapping for hashes handed out by list_models: ModelSummary omits created_at, so those hashes are per-call placeholders; without the mapping, get_model/list_snapshots could not resolve them even for fresh events, which broke batch_platforms (and with it the platform filter) from any fresh client. Regression tests age server-side events 30 days and drive a cache-free second client through all four behaviors, plus the fresh-event hash-resolution case. Co-Authored-By: Claude Fable 5 --- src/model_ledger/backends/http.py | 31 ++-- .../test_http_ledger_integration.py | 152 ++++++++++++++++++ 2 files changed, 173 insertions(+), 10 deletions(-) diff --git a/src/model_ledger/backends/http.py b/src/model_ledger/backends/http.py index 01cebde..c60fc6d 100644 --- a/src/model_ledger/backends/http.py +++ b/src/model_ledger/backends/http.py @@ -118,16 +118,20 @@ def list_models(self, **filters: str) -> list[ModelRef]: data = resp.json() models = [] for m in data.get("models", []): - models.append( - ModelRef( - name=m["name"], - owner=m.get("owner") or "unknown", - model_type=m.get("model_type") or "unknown", - tier="unclassified", - purpose="", - status=m.get("status") or "active", - ) + ref = ModelRef( + name=m["name"], + owner=m.get("owner") or "unknown", + model_type=m.get("model_type") or "unknown", + tier="unclassified", + purpose="", + status=m.get("status") or "active", ) + # ModelSummary omits created_at, so this hash is a client-local + # placeholder that changes on every call. Cache the name mapping + # so hashes handed out here stay resolvable by get_model / + # list_snapshots within this process (e.g. batch_platforms). + self._hash_to_name[ref.model_hash] = ref.name + models.append(ref) return models def update_model(self, model: ModelRef) -> None: @@ -190,7 +194,14 @@ def list_snapshots(self, model_hash: str, **filters: str) -> list[Snapshot]: model = self.get_model(model_hash) if not model: return [] - params: dict[str, Any] = {"model_name": model.name, "limit": 10000} + # The changelog tool defaults to a 7-day window when no bounds are + # given; list_snapshots must return the model's FULL history, so pass + # an explicit all-time lower bound. + params: dict[str, Any] = { + "model_name": model.name, + "limit": 10000, + "since": "1970-01-01T00:00:00+00:00", + } if "event_type" in filters: params["event_type"] = filters["event_type"] resp = self._client.get("/changelog", params=params) diff --git a/tests/test_backends/test_http_ledger_integration.py b/tests/test_backends/test_http_ledger_integration.py index c2e71a1..203b7aa 100644 --- a/tests/test_backends/test_http_ledger_integration.py +++ b/tests/test_backends/test_http_ledger_integration.py @@ -14,6 +14,8 @@ from __future__ import annotations +from datetime import timedelta + import pytest pytest.importorskip("httpx") @@ -132,6 +134,156 @@ def test_get_snapshot_unknown_hash_returns_none(self, backend): assert backend.get_snapshot("f" * 32) is None +def _make_stack(): + """A server with an inspectable backend plus an HTTP-backed Ledger.""" + from model_ledger.backends.ledger_memory import InMemoryLedgerBackend + + server_backend = InMemoryLedgerBackend() + server_app = create_app(backend=server_backend) + b = HttpLedgerBackend("http://testserver") + b._client.close() + b._client = TestClient(server_app) + return server_backend, server_app, b, Ledger(backend=b) + + +def _fresh_client(server_app): + """A second client process: no local caches, same server.""" + b = HttpLedgerBackend("http://testserver") + b._client.close() + b._client = TestClient(server_app) + return b, Ledger(backend=b) + + +def _age_server_events(server_backend, days=30): + """Rewind server-side event timestamps, keeping content hashes consistent + (in production an old snapshot's hash reflects its real timestamp).""" + from model_ledger.core.ledger_models import _compute_snapshot_hash + + for s in server_backend._snapshots: + object.__setattr__(s, "timestamp", s.timestamp - timedelta(days=days)) + object.__setattr__( + s, "snapshot_hash", _compute_snapshot_hash(s.model_hash, s.timestamp, s.payload) + ) + + +class TestOldEventsVisibleOverHttp: + """The changelog tool defaults to a 7-day window when no bounds are + given. ``list_snapshots`` must fetch the full history, or every SDK + behavior built on it silently breaks for models older than a week.""" + + def test_list_snapshots_sees_month_old_events(self): + server_backend, server_app, _, ledger = _make_stack() + ledger.register( + name="old-model", owner="risk", model_type="ml_model", tier="low", purpose="p" + ) + _age_server_events(server_backend) + + fresh, _ = _fresh_client(server_app) + ref = fresh.get_model_by_name("old-model") + assert len(fresh.list_snapshots(ref.model_hash)) >= 1 + + def test_latest_snapshot_and_tag_for_old_model(self): + server_backend, server_app, _, ledger = _make_stack() + ledger.register( + name="old-model", owner="risk", model_type="ml_model", tier="low", purpose="p" + ) + _age_server_events(server_backend) + + fresh, fresh_ledger = _fresh_client(server_app) + ref = fresh.get_model_by_name("old-model") + assert fresh.latest_snapshot(ref.model_hash) is not None + # Ledger.tag resolves latest_snapshot — must not raise for a + # perfectly healthy month-old model. + fresh_ledger.tag("old-model", "v1") + + def test_get_snapshot_reconstructs_month_old_snapshot(self): + server_backend, server_app, _, ledger = _make_stack() + ledger.register( + name="old-model", owner="risk", model_type="ml_model", tier="low", purpose="p" + ) + _age_server_events(server_backend) + old_server_hash = server_backend._snapshots[0].snapshot_hash + + fresh, _ = _fresh_client(server_app) + fresh.get_model_by_name("old-model") # populate name cache + got = fresh.get_snapshot(old_server_hash) + assert got is not None + assert got.snapshot_hash == old_server_hash + + def test_reregistration_of_old_model_is_idempotent_with_real_event_id(self): + from model_ledger.tools.record import record + from model_ledger.tools.schemas import RecordInput + + server_backend, server_app, _, ledger = _make_stack() + ledger.register( + name="old-model", owner="risk", model_type="ml_model", tier="low", purpose="p" + ) + _age_server_events(server_backend) + + _, fresh_ledger = _fresh_client(server_app) + out = record( + RecordInput( + model_name="old-model", + event="registered", + payload={}, + actor="ci", + owner="risk", + model_type="ml_model", + purpose="p", + ), + fresh_ledger, + ) + registered = [s for s in server_backend._snapshots if s.event_type == "registered"] + assert len(registered) == 1 # idempotent even when the event is old + assert out.is_new_model is False + # The returned event_id identifies a real server-side event. + assert out.event_id in {s.snapshot_hash for s in server_backend._snapshots} + + def test_platform_filter_finds_month_old_model(self): + from model_ledger.tools.query import query + from model_ledger.tools.schemas import QueryInput + + server_backend, server_app, _, ledger = _make_stack() + ledger.register( + name="old-mlflow", owner="o", model_type="ml_model", tier="low", purpose="p" + ) + ledger.record( + "old-mlflow", + event="discovered", + payload={"platform": "mlflow"}, + actor="c", + source="mlflow", + ) + _age_server_events(server_backend) + + _, fresh_ledger = _fresh_client(server_app) + res = query(QueryInput(platform="mlflow"), fresh_ledger) + assert res.total == 1 + assert res.models[0].name == "old-mlflow" + + def test_platform_filter_finds_fresh_model_from_fresh_client(self): + """Hash-resolution regression: hashes handed out by list_models must + be resolvable by the same backend (list_snapshots via get_model), + or batch_platforms sees zero snapshots for every model.""" + from model_ledger.tools.query import query + from model_ledger.tools.schemas import QueryInput + + _, server_app, _, ledger = _make_stack() + ledger.register( + name="fresh-mlflow", owner="o", model_type="ml_model", tier="low", purpose="p" + ) + ledger.record( + "fresh-mlflow", + event="discovered", + payload={"platform": "mlflow"}, + actor="c", + source="mlflow", + ) + _, fresh_ledger = _fresh_client(server_app) + res = query(QueryInput(platform="mlflow"), fresh_ledger) + assert res.total == 1 + + class TestServerRejectsEmptyModelName: """Defense in depth: old clients that still post ``model_name: ""`` must fail loudly instead of silently corrupting the ledger.""" From a757f298b2d7f3fc93c75c1d880d44b56334b78b Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:43:34 -0700 Subject: [PATCH 16/22] fix(http): registration payload, tier, and actor reach the server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Over HttpLedgerBackend the caller's registration payload was silently dropped: save_model posted payload={}, and the SDK's follow-up append_snapshot(registered) — the only carrier of the payload — was exactly the request the redundant-registration skip suppressed. Tier and actor were lost the same way (every remote registration landed as tier="unclassified", actor="user"). Ledger.register() now dispatches to an optional backend register_model hook when the backend defines one; HttpLedgerBackend implements it as a single POST /record carrying owner, model_type, purpose, tier, actor, and the caller's payload, then adopts the server's canonical hash as before. Backends without the hook keep the existing two-step save_model + append_snapshot path unchanged. RecordInput gains an optional tier field (REST body and MCP tool arg, additive); the record tool passes it through to register(). Co-Authored-By: Claude Fable 5 --- src/model_ledger/backends/http.py | 24 ++++++- src/model_ledger/mcp/server.py | 4 ++ src/model_ledger/sdk/ledger.py | 41 +++++++----- src/model_ledger/tools/record.py | 2 +- src/model_ledger/tools/schemas.py | 1 + .../test_http_ledger_integration.py | 62 +++++++++++++++++++ 6 files changed, 113 insertions(+), 21 deletions(-) diff --git a/src/model_ledger/backends/http.py b/src/model_ledger/backends/http.py index c60fc6d..500937f 100644 --- a/src/model_ledger/backends/http.py +++ b/src/model_ledger/backends/http.py @@ -46,7 +46,20 @@ def __init__(self, base_url: str, headers: dict[str, str] | None = None) -> None # ── Models ── - def save_model(self, model: ModelRef) -> None: + def register_model( + self, + model: ModelRef, + *, + payload: dict[str, Any] | None = None, + actor: str = "system", + ) -> None: + """Register a model server-side in a single POST /record. + + ``Ledger.register()`` dispatches here instead of the two-step + save_model + append_snapshot, so the caller's registration payload, + tier, and actor reach the server (the server's record tool logs the + single ``registered`` event itself). + """ resp = self._client.post( "/record", json={ @@ -55,7 +68,9 @@ def save_model(self, model: ModelRef) -> None: "owner": model.owner, "model_type": model.model_type, "purpose": model.purpose, - "payload": {}, + "tier": model.tier, + "actor": actor, + "payload": payload or {}, }, ) # Fail loudly on HTTP errors rather than caching a model that was @@ -78,10 +93,13 @@ def save_model(self, model: ModelRef) -> None: model.model_hash = server_hash self._hash_to_name[server_hash] = model.name # The server's /record tool logs the "registered" event as part of - # registration — remember that so the SDK's follow-up + # registration — remember that so any follow-up # append_snapshot(registered) is not posted a second time. self._registered_hashes.add(server_hash) + def save_model(self, model: ModelRef) -> None: + self.register_model(model) + def get_model(self, model_hash: str) -> ModelRef | None: name = self._hash_to_name.get(model_hash) if name is not None: diff --git a/src/model_ledger/mcp/server.py b/src/model_ledger/mcp/server.py index cf87387..757723f 100644 --- a/src/model_ledger/mcp/server.py +++ b/src/model_ledger/mcp/server.py @@ -107,6 +107,7 @@ def record( owner: str | None = None, model_type: str | None = None, purpose: str | None = None, + tier: str | None = None, ) -> dict: """Register a new model or record an event on an existing model. @@ -121,6 +122,7 @@ def record( owner=owner, model_type=model_type, purpose=purpose, + tier=tier, ) return _record(inp, ledger).model_dump(mode="json") @@ -354,6 +356,7 @@ def record( owner: str | None = None, model_type: str | None = None, purpose: str | None = None, + tier: str | None = None, ) -> dict: """Register a new model or record an event on an existing model. @@ -370,6 +373,7 @@ def record( "owner": owner, "model_type": model_type, "purpose": purpose, + "tier": tier, }, ) return resp.json() # type: ignore[no-any-return] diff --git a/src/model_ledger/sdk/ledger.py b/src/model_ledger/sdk/ledger.py index b52fa04..a2fdd66 100644 --- a/src/model_ledger/sdk/ledger.py +++ b/src/model_ledger/sdk/ledger.py @@ -170,24 +170,31 @@ def register( status=status, metadata=metadata or {}, ) - self._backend.save_model(model) - event_payload: dict[str, Any] = { - "name": name, - "owner": owner, - "tier": tier, - "purpose": purpose, - "model_origin": model_origin, - } - if payload: - event_payload.update(payload) - self._backend.append_snapshot( - Snapshot( - model_hash=model.model_hash, - actor=actor, - event_type="registered", - payload=event_payload, + register_model = getattr(self._backend, "register_model", None) + if callable(register_model): + # Pass-through backends (e.g. HttpLedgerBackend) register in a + # single call so the payload, tier, and actor reach the remote + # server, which logs the single "registered" event itself. + register_model(model, payload=payload or {}, actor=actor) + else: + self._backend.save_model(model) + event_payload: dict[str, Any] = { + "name": name, + "owner": owner, + "tier": tier, + "purpose": purpose, + "model_origin": model_origin, + } + if payload: + event_payload.update(payload) + self._backend.append_snapshot( + Snapshot( + model_hash=model.model_hash, + actor=actor, + event_type="registered", + payload=event_payload, + ) ) - ) self._name_cache[name] = model return model diff --git a/src/model_ledger/tools/record.py b/src/model_ledger/tools/record.py index 7bcf53d..7632e7e 100644 --- a/src/model_ledger/tools/record.py +++ b/src/model_ledger/tools/record.py @@ -26,7 +26,7 @@ def record(input: RecordInput, ledger: Ledger) -> RecordOutput: name=input.model_name, owner=input.owner or "unknown", model_type=input.model_type or "unknown", - tier="unclassified", + tier=input.tier or "unclassified", purpose=input.purpose or "", actor=input.actor, payload=input.payload or None, diff --git a/src/model_ledger/tools/schemas.py b/src/model_ledger/tools/schemas.py index d10ed0d..4b7d802 100644 --- a/src/model_ledger/tools/schemas.py +++ b/src/model_ledger/tools/schemas.py @@ -68,6 +68,7 @@ class RecordInput(BaseModel): owner: str | None = None model_type: str | None = None purpose: str | None = None + tier: str | None = None class RecordOutput(BaseModel): diff --git a/tests/test_backends/test_http_ledger_integration.py b/tests/test_backends/test_http_ledger_integration.py index 203b7aa..df0c0cb 100644 --- a/tests/test_backends/test_http_ledger_integration.py +++ b/tests/test_backends/test_http_ledger_integration.py @@ -284,6 +284,68 @@ def test_platform_filter_finds_fresh_model_from_fresh_client(self): assert res.total == 1 +class TestRegistrationFidelityOverHttp: + """Registration over HTTP must carry the same information as a local + backend: the caller's payload lands on the single registered event, + and the tier lands on the model row.""" + + def test_registration_payload_lands_on_registered_event(self): + from model_ledger.tools.record import record + from model_ledger.tools.schemas import RecordInput + + server_backend, _, _, ledger = _make_stack() + record( + RecordInput( + model_name="m1", + event="registered", + payload={"framework": "xgboost", "version": "2.1"}, + actor="ci", + owner="risk", + model_type="ml_model", + purpose="scoring", + ), + ledger, + ) + registered = [s for s in server_backend._snapshots if s.event_type == "registered"] + assert len(registered) == 1 + assert registered[0].payload.get("framework") == "xgboost" + assert registered[0].payload.get("version") == "2.1" + + def test_register_payload_kwarg_lands_on_registered_event(self): + server_backend, _, _, ledger = _make_stack() + ledger.register( + name="m2", + owner="risk", + model_type="ml_model", + tier="low", + purpose="p", + payload={"framework": "lightgbm"}, + ) + registered = [s for s in server_backend._snapshots if s.event_type == "registered"] + assert len(registered) == 1 + assert registered[0].payload.get("framework") == "lightgbm" + + def test_tier_survives_http_registration(self): + server_backend, _, _, ledger = _make_stack() + ledger.register( + name="tiered", owner="risk", model_type="ml_model", tier="high", purpose="p" + ) + assert server_backend.get_model_by_name("tiered").tier == "high" + + def test_actor_survives_http_registration(self): + server_backend, _, _, ledger = _make_stack() + ledger.register( + name="acted", + owner="risk", + model_type="ml_model", + tier="low", + purpose="p", + actor="pipeline-bot", + ) + registered = [s for s in server_backend._snapshots if s.event_type == "registered"] + assert registered[0].actor == "pipeline-bot" + + class TestServerRejectsEmptyModelName: """Defense in depth: old clients that still post ``model_name: ""`` must fail loudly instead of silently corrupting the ledger.""" From 9873c100a60e059b73096f57b3e5f0fffe4a0d61 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:44:42 -0700 Subject: [PATCH 17/22] fix(connectors): accept sqlite3.Row and other keys()-style rows The dict-row guard used hasattr(row, "get") as a proxy for "addressable by column name", which rejected the stdlib's own mapping-style row type: sqlite3.Row supports keys() and row["col"] but has no .get(), so configurations that discovered nodes on 0.7.12 started raising the tuple-row TypeError. Rows with keys() are now materialized to dicts; only true positional rows are rejected. The docs example now recommends row_factory = sqlite3.Row. Co-Authored-By: Claude Fable 5 --- docs/guides/connectors.md | 10 ++--- src/model_ledger/connectors/sql.py | 19 ++++++--- tests/test_connectors/test_sql_connector.py | 45 +++++++++++++++++++++ 3 files changed, 62 insertions(+), 12 deletions(-) diff --git a/docs/guides/connectors.md b/docs/guides/connectors.md index 562311a..a46196d 100644 --- a/docs/guides/connectors.md +++ b/docs/guides/connectors.md @@ -38,19 +38,17 @@ ledger.add(etl_jobs.discover()) ledger.connect() # links ETL outputs to model inputs automatically ``` -!!! note "The connection must return dict-style rows" +!!! note "The connection must return name-addressable rows" `sql_connector` addresses row values by column name (`row["owner"]`), so the connection's `execute()` must return mappings, not raw DB-API tuples — tuple - rows raise a `TypeError` at discovery. For `sqlite3`, set a dict row factory; - for most other drivers, use the `DictCursor` equivalent: + rows raise a `TypeError` at discovery. For `sqlite3` the stdlib row type is + enough; for most other drivers, use the `DictCursor` equivalent: ```python import sqlite3 conn = sqlite3.connect("registry.db") - conn.row_factory = lambda cursor, row: { - d[0]: row[i] for i, d in enumerate(cursor.description) - } + conn.row_factory = sqlite3.Row ``` ## REST APIs diff --git a/src/model_ledger/connectors/sql.py b/src/model_ledger/connectors/sql.py index 06c4819..f9d49ad 100644 --- a/src/model_ledger/connectors/sql.py +++ b/src/model_ledger/connectors/sql.py @@ -151,12 +151,19 @@ def discover(self) -> list[DataNode]: nodes: list[DataNode] = [] for row in rows: if not hasattr(row, "get"): - raise TypeError( - "sql_connector requires dict-style rows (mappings addressable " - f"by column name), got {type(row).__name__}. Configure the " - "connection to return mappings — e.g. set a dict row_factory " - "on sqlite3, or use your driver's DictCursor." - ) + if hasattr(row, "keys"): + # Name-addressable rows without the full dict API + # (e.g. sqlite3.Row has keys() and row["col"] but no + # .get()/.items()) — materialize a real dict. + row = dict(row) + else: + raise TypeError( + "sql_connector requires dict-style rows (mappings addressable " + f"by column name), got {type(row).__name__}. Configure the " + "connection to return mappings — e.g. set " + "row_factory = sqlite3.Row (or a dict row_factory) on " + "sqlite3, or use your driver's DictCursor." + ) nodes.append(self._to_node(row)) return nodes diff --git a/tests/test_connectors/test_sql_connector.py b/tests/test_connectors/test_sql_connector.py index fc4ebf1..8468bf8 100644 --- a/tests/test_connectors/test_sql_connector.py +++ b/tests/test_connectors/test_sql_connector.py @@ -382,3 +382,48 @@ def test_dict_row_factory_works(self, tmp_path): assert nodes[0].name == "model_a" assert nodes[0].metadata["owner"] == "alice" conn.close() + + def test_sqlite3_row_factory_works(self, tmp_path): + """sqlite3.Row is name-addressable (keys() + row["col"]) but has no + .get() — the stdlib's own mapping-style row type must be accepted.""" + import sqlite3 + + db = tmp_path / "registry.db" + conn = sqlite3.connect(db) + conn.row_factory = sqlite3.Row + conn.execute("CREATE TABLE models (name TEXT, owner TEXT)") + conn.execute("INSERT INTO models VALUES ('model_a', 'alice')") + conn.commit() + + c = sql_connector( + name="registry", + connection=conn, + query="SELECT name, owner FROM models", + name_column="name", + ) + nodes = c.discover() + assert nodes[0].name == "model_a" + # Auto-metadata mode consumes unmapped columns from the row too. + assert nodes[0].metadata["owner"] == "alice" + conn.close() + + def test_sqlite3_row_with_empty_metadata_columns(self, tmp_path): + import sqlite3 + + db = tmp_path / "registry.db" + conn = sqlite3.connect(db) + conn.row_factory = sqlite3.Row + conn.execute("CREATE TABLE models (name TEXT)") + conn.execute("INSERT INTO models VALUES ('m1')") + conn.commit() + + c = sql_connector( + name="reg", + connection=conn, + query="SELECT name FROM models", + name_column="name", + metadata_columns={}, + ) + nodes = c.discover() + assert nodes[0].name == "m1" + conn.close() From decf6594d07d93d760bc8124243dd274ea9f6d7b Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:46:15 -0700 Subject: [PATCH 18/22] fix(backends): validate_backend duck-checks core methods instead of full-protocol isinstance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runtime_checkable isinstance check demanded all 14 protocol method names, so a handwritten partial backend (no list_snapshots_before or tag methods) that served create_app() traffic fine on 0.7.12 died with TypeError at construction — a compatibility break for third-party backends. The guard now rejects the two intended mistakes (a Ledger instance, a path string/PathLike) with the same hints and otherwise requires only a core-method subset, so previously-working partial backends keep constructing. Co-Authored-By: Claude Fable 5 --- src/model_ledger/backends/ledger_protocol.py | 41 ++++++++++++----- tests/test_mcp/test_server.py | 42 +++++++++++++++++ tests/test_rest/test_app.py | 47 ++++++++++++++++++++ 3 files changed, 120 insertions(+), 10 deletions(-) diff --git a/src/model_ledger/backends/ledger_protocol.py b/src/model_ledger/backends/ledger_protocol.py index 1de3597..4097e55 100644 --- a/src/model_ledger/backends/ledger_protocol.py +++ b/src/model_ledger/backends/ledger_protocol.py @@ -2,33 +2,54 @@ from __future__ import annotations +import os from datetime import datetime from typing import Protocol, runtime_checkable from model_ledger.core.ledger_models import ModelRef, Snapshot, Tag +# The duck-typing floor for validate_backend. Deliberately a SUBSET of the +# protocol: handwritten third-party backends that skip optional methods +# (list_snapshots_before, tag methods) must keep constructing — the goal is +# to catch obviously-wrong arguments early, not to enforce completeness. +_CORE_BACKEND_METHODS = ( + "save_model", + "get_model_by_name", + "list_models", + "append_snapshot", + "list_snapshots", +) + def validate_backend(backend: object | None) -> None: - """Raise ``TypeError`` early if ``backend`` is not a ``LedgerBackend``. + """Raise ``TypeError`` early if ``backend`` is clearly not a ``LedgerBackend``. Catches the common mistake of passing a ``Ledger`` (or a database path) where a storage backend is expected — which would otherwise fail deep - inside the first operation with a confusing ``AttributeError``. + inside the first operation with a confusing ``AttributeError``. Uses a + core-method duck check rather than full-protocol ``isinstance`` so + partial custom backends are not rejected. """ - if backend is None or isinstance(backend, LedgerBackend): + if backend is None: return from model_ledger.sdk.ledger import Ledger - hint = "" if isinstance(backend, Ledger): - hint = ( + raise TypeError( + "backend must implement the LedgerBackend protocol; got Ledger" " — pass the Ledger's storage backend (e.g. SQLiteLedgerBackend), not the Ledger itself" ) - elif isinstance(backend, str): - hint = " — construct a backend from the path first (e.g. SQLiteLedgerBackend(path))" - raise TypeError( - f"backend must implement the LedgerBackend protocol; got {type(backend).__name__}{hint}" - ) + if isinstance(backend, str | os.PathLike): + raise TypeError( + f"backend must implement the LedgerBackend protocol; got {type(backend).__name__}" + " — construct a backend from the path first (e.g. SQLiteLedgerBackend(path))" + ) + missing = [m for m in _CORE_BACKEND_METHODS if not callable(getattr(backend, m, None))] + if missing: + raise TypeError( + f"backend must implement the LedgerBackend protocol; got {type(backend).__name__}" + f" (missing {', '.join(missing)})" + ) @runtime_checkable diff --git a/tests/test_mcp/test_server.py b/tests/test_mcp/test_server.py index e16600d..565fbe7 100644 --- a/tests/test_mcp/test_server.py +++ b/tests/test_mcp/test_server.py @@ -227,3 +227,45 @@ def test_accepts_real_backend(self): server = create_server(backend=InMemoryLedgerBackend()) assert server is not None + + def test_accepts_partial_custom_backend(self): + """Duck typing, not full-protocol isinstance — partial third-party + backends that worked on 0.7.12 must keep working.""" + from model_ledger.backends.ledger_memory import InMemoryLedgerBackend + from model_ledger.mcp.server import create_server + + class PartialBackend: + def __init__(self): + self._i = InMemoryLedgerBackend() + + def save_model(self, m): + return self._i.save_model(m) + + def get_model(self, h): + return self._i.get_model(h) + + def get_model_by_name(self, n): + return self._i.get_model_by_name(n) + + def list_models(self, **f): + return self._i.list_models(**f) + + def append_snapshot(self, s): + return self._i.append_snapshot(s) + + def list_snapshots(self, h, **f): + return self._i.list_snapshots(h, **f) + + def latest_snapshot(self, h, tag=None): + return self._i.latest_snapshot(h, tag) + + # deliberately missing: update_model, get_snapshot, + # list_snapshots_before, set_tag, get_tag, list_tags + + assert create_server(backend=PartialBackend()) is not None + + def test_rejects_object_missing_core_methods(self): + from model_ledger.mcp.server import create_server + + with pytest.raises(TypeError, match="LedgerBackend"): + create_server(backend=object()) diff --git a/tests/test_rest/test_app.py b/tests/test_rest/test_app.py index b0a5e0a..3982dfb 100644 --- a/tests/test_rest/test_app.py +++ b/tests/test_rest/test_app.py @@ -280,3 +280,50 @@ def test_accepts_real_backend(self): app = create_app(backend=InMemoryLedgerBackend()) assert TestClient(app).get("/overview").status_code == 200 + + def test_accepts_partial_custom_backend(self): + """Duck typing, not full-protocol isinstance: a handwritten backend + that skips optional methods (list_snapshots_before, tags) worked on + 0.7.12 and must keep working.""" + from model_ledger.backends.ledger_memory import InMemoryLedgerBackend + + class PartialBackend: + def __init__(self): + self._i = InMemoryLedgerBackend() + + def save_model(self, m): + return self._i.save_model(m) + + def get_model(self, h): + return self._i.get_model(h) + + def get_model_by_name(self, n): + return self._i.get_model_by_name(n) + + def list_models(self, **f): + return self._i.list_models(**f) + + def update_model(self, m): + return self._i.update_model(m) + + def append_snapshot(self, s): + return self._i.append_snapshot(s) + + def get_snapshot(self, h): + return self._i.get_snapshot(h) + + def list_snapshots(self, h, **f): + return self._i.list_snapshots(h, **f) + + def latest_snapshot(self, h, tag=None): + return self._i.latest_snapshot(h, tag) + + # deliberately missing: list_snapshots_before, set_tag, + # get_tag, list_tags + + app = create_app(backend=PartialBackend()) + assert TestClient(app).get("/overview").status_code == 200 + + def test_rejects_object_missing_core_methods(self): + with pytest.raises(TypeError, match="LedgerBackend"): + create_app(backend=object()) From 9300c91c73c43238e2659f565002c05dfcc3927c Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:47:59 -0700 Subject: [PATCH 19/22] test(introspect): dispatch tests execute without the ML libraries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dispatch regression tests importorskipped numpy/sklearn/xgboost/ lightgbm, and neither CI nor the default dev install carries them — all four tests skipped everywhere, leaving the dispatch fix with zero executed coverage in the release pipeline. Add a stub-module layer: minimal sklearn/xgboost/lightgbm modules with the real inheritance shape (wrapper estimators subclass BaseEstimator) installed via monkeypatch, exercising registry.find() and SklearnIntrospector.can_handle in every environment, plus a __module__ = None safety case. With the can_handle fix reverted, the stub tests fail in a bare venv (verified). The real-library tests remain and still run where the ML extras are installed. Co-Authored-By: Claude Fable 5 --- tests/test_introspect/test_dispatch.py | 183 ++++++++++++++++++++----- 1 file changed, 145 insertions(+), 38 deletions(-) diff --git a/tests/test_introspect/test_dispatch.py b/tests/test_introspect/test_dispatch.py index e721a15..fd2f531 100644 --- a/tests/test_introspect/test_dispatch.py +++ b/tests/test_introspect/test_dispatch.py @@ -7,16 +7,25 @@ without an explicit rejection in ``SklearnIntrospector.can_handle`` the generic introspector shadows the xgboost one (and lightgbm only escaped by alphabetical luck). + +Two layers of coverage: + +- ``TestDispatchWithStubs`` installs minimal stub ``sklearn``/``xgboost``/ + ``lightgbm`` modules, so the dispatch logic EXECUTES in every environment + (CI installs no ML libraries — importorskip-only tests would never run). +- ``TestDispatchWithRealLibraries`` drives the same assertions against the + real libraries where they are installed. """ from __future__ import annotations +import sys +import types + import pytest from model_ledger.introspect.registry import get_registry, reset_registry -np = pytest.importorskip("numpy") - @pytest.fixture(autouse=True) def fresh_registry(): @@ -25,58 +34,156 @@ def fresh_registry(): reset_registry() +# --------------------------------------------------------------------------- +# Stub-module layer: executes everywhere, no ML dependencies required. +# --------------------------------------------------------------------------- + + +@pytest.fixture +def stub_ml_libs(monkeypatch): + """Minimal sklearn/xgboost/lightgbm stubs with the real inheritance shape: + wrapper-library estimators subclass sklearn's BaseEstimator.""" + sklearn = types.ModuleType("sklearn") + sklearn_base = types.ModuleType("sklearn.base") + + class BaseEstimator: + pass + + sklearn_base.BaseEstimator = BaseEstimator + sklearn.base = sklearn_base + + xgboost = types.ModuleType("xgboost") + + class XGBModel(BaseEstimator): + pass + + class XGBoosterStub: + pass + + xgboost.XGBModel = XGBModel + xgboost.Booster = XGBoosterStub + + lightgbm = types.ModuleType("lightgbm") + + class LGBMModel(BaseEstimator): + pass + + class LGBBoosterStub: + pass + + lightgbm.LGBMModel = LGBMModel + lightgbm.Booster = LGBBoosterStub + + for name, mod in { + "sklearn": sklearn, + "sklearn.base": sklearn_base, + "xgboost": xgboost, + "lightgbm": lightgbm, + }.items(): + monkeypatch.setitem(sys.modules, name, mod) + + return types.SimpleNamespace(sklearn=sklearn, xgboost=xgboost, lightgbm=lightgbm) + + +def _make_estimator(base, class_name, module): + """An estimator instance whose class lives in ``module`` (like the real + xgboost.sklearn.XGBClassifier / sklearn.linear_model.LogisticRegression).""" + cls = type(class_name, (base,), {"__module__": module}) + return cls() + + +class TestDispatchWithStubs: + def test_xgboost_wrapper_dispatches_to_xgboost(self, stub_ml_libs): + model = _make_estimator(stub_ml_libs.xgboost.XGBModel, "XGBClassifier", "xgboost.sklearn") + assert get_registry().find(model).name == "xgboost" + + def test_lightgbm_wrapper_dispatches_to_lightgbm(self, stub_ml_libs): + model = _make_estimator( + stub_ml_libs.lightgbm.LGBMModel, "LGBMClassifier", "lightgbm.sklearn" + ) + assert get_registry().find(model).name == "lightgbm" + + def test_plain_estimator_dispatches_to_sklearn(self, stub_ml_libs): + model = _make_estimator( + stub_ml_libs.sklearn.base.BaseEstimator, + "LogisticRegression", + "sklearn.linear_model", + ) + assert get_registry().find(model).name == "sklearn" + + def test_sklearn_can_handle_rejects_wrapper_modules_regardless_of_order(self, stub_ml_libs): + """Order-independence: even if a new introspector name sorted the + sklearn introspector first, its can_handle must refuse + wrapper-library objects.""" + from model_ledger.introspect.sklearn import SklearnIntrospector + + model = _make_estimator(stub_ml_libs.xgboost.XGBModel, "XGBClassifier", "xgboost.sklearn") + assert SklearnIntrospector().can_handle(model) is False + + def test_none_module_is_safe_and_goes_to_sklearn(self, stub_ml_libs): + """A BaseEstimator subclass with ``__module__ = None`` must not crash + the module-root check.""" + model = _make_estimator(stub_ml_libs.sklearn.base.BaseEstimator, "Weird", "x") + type(model).__module__ = None + assert get_registry().find(model).name == "sklearn" + + +# --------------------------------------------------------------------------- +# Real-library layer: runs where the ML extras are installed. +# --------------------------------------------------------------------------- + + def _training_data(): + np = pytest.importorskip("numpy") X = np.array([[1.0, 2.0], [3.0, 4.0], [5.0, 6.0], [7.0, 8.0]]) y = np.array([0, 1, 0, 1]) return X, y -def test_xgboost_sklearn_api_dispatches_to_xgboost(): - xgb = pytest.importorskip("xgboost") - pytest.importorskip("sklearn") - - X, y = _training_data() - model = xgb.XGBClassifier(n_estimators=2, max_depth=2).fit(X, y) - - intro = get_registry().find(model) - assert intro.name == "xgboost" - - result = intro.introspect(model) - assert result.introspector == "xgboost" - assert result.framework == "xgboost" +class TestDispatchWithRealLibraries: + def test_xgboost_sklearn_api_dispatches_to_xgboost(self): + xgb = pytest.importorskip("xgboost") + pytest.importorskip("sklearn") + X, y = _training_data() + model = xgb.XGBClassifier(n_estimators=2, max_depth=2).fit(X, y) -def test_lightgbm_sklearn_api_dispatches_to_lightgbm(): - lgb = pytest.importorskip("lightgbm") - pytest.importorskip("sklearn") + intro = get_registry().find(model) + assert intro.name == "xgboost" - X, y = _training_data() - model = lgb.LGBMClassifier(n_estimators=2, min_child_samples=1).fit(X, y) + result = intro.introspect(model) + assert result.introspector == "xgboost" + assert result.framework == "xgboost" - intro = get_registry().find(model) - assert intro.name == "lightgbm" + def test_lightgbm_sklearn_api_dispatches_to_lightgbm(self): + lgb = pytest.importorskip("lightgbm") + pytest.importorskip("sklearn") + X, y = _training_data() + model = lgb.LGBMClassifier(n_estimators=2, min_child_samples=1).fit(X, y) -def test_plain_sklearn_estimator_still_dispatches_to_sklearn(): - sklearn = pytest.importorskip("sklearn") # noqa: F841 - from sklearn.linear_model import LogisticRegression + intro = get_registry().find(model) + assert intro.name == "lightgbm" - X, y = _training_data() - model = LogisticRegression().fit(X, y) + def test_plain_sklearn_estimator_still_dispatches_to_sklearn(self): + pytest.importorskip("sklearn") + from sklearn.linear_model import LogisticRegression - intro = get_registry().find(model) - assert intro.name == "sklearn" + X, y = _training_data() + model = LogisticRegression().fit(X, y) + intro = get_registry().find(model) + assert intro.name == "sklearn" -def test_sklearn_can_handle_rejects_wrapper_modules_regardless_of_order(): - """Order-independence: even if a new introspector name sorted the sklearn - introspector first, its can_handle must refuse wrapper-library objects.""" - pytest.importorskip("sklearn") - xgb = pytest.importorskip("xgboost") + def test_sklearn_can_handle_rejects_wrapper_modules_regardless_of_order(self): + """Order-independence: even if a new introspector name sorted the sklearn + introspector first, its can_handle must refuse wrapper-library objects.""" + pytest.importorskip("sklearn") + xgb = pytest.importorskip("xgboost") - from model_ledger.introspect.sklearn import SklearnIntrospector + from model_ledger.introspect.sklearn import SklearnIntrospector - X, y = _training_data() - model = xgb.XGBClassifier(n_estimators=2, max_depth=2).fit(X, y) + X, y = _training_data() + model = xgb.XGBClassifier(n_estimators=2, max_depth=2).fit(X, y) - assert SklearnIntrospector().can_handle(model) is False + assert SklearnIntrospector().can_handle(model) is False From 086a60443b3cc9bf9ef2c96b3854ce3e933f41d3 Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:50:21 -0700 Subject: [PATCH 20/22] =?UTF-8?q?revert:=20keep=200.7.13=20patch-safe=20?= =?UTF-8?q?=E2=80=94=20restore=20excel=20extra=20and=20export=20default=20?= =?UTF-8?q?filename?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing the excel extra broke `model-ledger[excel]` pins within a patch version (uv hard-errors on unknown extras), and changing the export default artifact from audit_pack to audit_pack.html broke scripts consuming the old default path. Restore both: the extra is marked deprecated (removal planned for 0.8.0) and the export help text keeps the corrected single-file wording. Co-Authored-By: Claude Fable 5 --- docs/installation.md | 1 + pyproject.toml | 4 ++++ src/model_ledger/cli/app.py | 2 +- 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/installation.md b/docs/installation.md index dd52607..57e8989 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -27,6 +27,7 @@ uv add model-ledger | `model-ledger[introspect-sklearn]` | scikit-learn introspector | extract algorithm/features from fitted models | | `model-ledger[introspect-xgboost]` | XGBoost introspector | " | | `model-ledger[introspect-lightgbm]` | LightGBM introspector | " | +| `model-ledger[excel]` | openpyxl | deprecated (unused since the scanner removal); kept for pin compatibility, removal planned for 0.8.0 | | `model-ledger[all]` | Snowflake + pandas + httpx | the common production set | Combine them: `pip install "model-ledger[mcp,rest-api,snowflake]"`. diff --git a/pyproject.toml b/pyproject.toml index 9c49cb7..72df97c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -43,6 +43,10 @@ dependencies = [ [project.optional-dependencies] cli = ["typer>=0.9", "rich>=13.0"] +# Deprecated: nothing imports openpyxl since the scanner module was removed +# (post-0.7.7). Kept so `model-ledger[excel]` pins keep resolving within +# 0.7.x; removal planned for 0.8.0. +excel = ["openpyxl>=3.1"] introspect-sklearn = ["scikit-learn>=1.0"] introspect-xgboost = ["xgboost>=1.7"] introspect-lightgbm = ["lightgbm>=3.0"] diff --git a/src/model_ledger/cli/app.py b/src/model_ledger/cli/app.py index 164d44f..7ac3bcc 100644 --- a/src/model_ledger/cli/app.py +++ b/src/model_ledger/cli/app.py @@ -374,7 +374,7 @@ def export_cmd( db: str = typer.Option(default=None, help="Path to the inventory database."), version: str | None = typer.Option(None, help="Version to export. Defaults to latest."), output: str = typer.Option( - "audit_pack.html", help="Output file path for the audit pack (a single HTML file)." + "audit_pack", help="Output file path for the audit pack (a single HTML file)." ), ) -> None: """Export an audit pack for a model version.""" From dd6039bcd8cb60aef0115a49f8e919f1fc0740ae Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:50:32 -0700 Subject: [PATCH 21/22] fix(tools): whitespace-only names rejected; trace total_nodes counts distinct models; public Ledger.backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review notes: - RecordInput.model_name rejects whitespace-only values (min_length=1 let " " register a near-invisible model); REST returns 422. - TraceOutput.total_nodes counts distinct models — a node reachable both upstream and downstream with direction="both" (e.g. a 2-cycle) was counted twice. - Ledger gains a public read-only backend property; tool functions use it instead of reaching into the private _backend attribute. Co-Authored-By: Claude Fable 5 --- src/model_ledger/rest/app.py | 2 +- src/model_ledger/sdk/ledger.py | 10 +++++++ src/model_ledger/tools/changelog.py | 2 +- src/model_ledger/tools/investigate.py | 2 +- src/model_ledger/tools/query.py | 6 ++--- src/model_ledger/tools/record.py | 6 ++--- src/model_ledger/tools/schemas.py | 10 ++++++- src/model_ledger/tools/tag.py | 2 +- src/model_ledger/tools/trace.py | 6 +++-- .../test_http_ledger_integration.py | 8 ++++++ tests/test_tools/test_trace.py | 27 +++++++++++++++++++ 11 files changed, 68 insertions(+), 13 deletions(-) diff --git a/src/model_ledger/rest/app.py b/src/model_ledger/rest/app.py index 97981de..62c084d 100644 --- a/src/model_ledger/rest/app.py +++ b/src/model_ledger/rest/app.py @@ -182,7 +182,7 @@ def list_tags_endpoint(model_name: str) -> TagListOutput: @app.get("/overview") def overview_endpoint() -> dict[str, Any]: models = ledger.list() - backend = ledger._backend + backend = ledger.backend if hasattr(backend, "count_all_snapshots"): total_events = backend.count_all_snapshots() else: diff --git a/src/model_ledger/sdk/ledger.py b/src/model_ledger/sdk/ledger.py index a2fdd66..c22a6be 100644 --- a/src/model_ledger/sdk/ledger.py +++ b/src/model_ledger/sdk/ledger.py @@ -77,6 +77,16 @@ def __init__(self, backend: LedgerBackend | None = None) -> None: self._cache_complete = False # True after bulk preload — skip individual lookups self._node_cache: list = [] # DataNodes from add() — reused by connect() + @property + def backend(self) -> LedgerBackend: + """The storage backend this ledger reads from and writes to. + + Public accessor for tool functions and integrations that dispatch on + optional backend capabilities (``batch_platforms``, ``changelog_page``, + ...) — avoids reaching into the private ``_backend`` attribute. + """ + return self._backend + @classmethod def from_sqlite(cls, db_path: str) -> Ledger: """Create a Ledger backed by a SQLite database. diff --git a/src/model_ledger/tools/changelog.py b/src/model_ledger/tools/changelog.py index 6bbe3d7..e883114 100644 --- a/src/model_ledger/tools/changelog.py +++ b/src/model_ledger/tools/changelog.py @@ -53,7 +53,7 @@ def changelog(input: ChangelogInput, ledger: Ledger) -> ChangelogOutput: model = ledger.get(input.model_name) model_hash = model.model_hash - backend = ledger._backend + backend = ledger.backend if hasattr(backend, "changelog_page"): events_dicts, total = backend.changelog_page( since=since, diff --git a/src/model_ledger/tools/investigate.py b/src/model_ledger/tools/investigate.py index 4407b18..6fc40c2 100644 --- a/src/model_ledger/tools/investigate.py +++ b/src/model_ledger/tools/investigate.py @@ -81,7 +81,7 @@ def investigate(input: InvestigateInput, ledger: Ledger) -> InvestigateOutput: now = datetime.now(timezone.utc) days_since_last_event = (now - latest_ts).days - backend = ledger._backend + backend = ledger.backend try: if hasattr(backend, "batch_dependencies"): deps = backend.batch_dependencies(model.model_hash) diff --git a/src/model_ledger/tools/query.py b/src/model_ledger/tools/query.py index dc3143e..ef4e511 100644 --- a/src/model_ledger/tools/query.py +++ b/src/model_ledger/tools/query.py @@ -42,7 +42,7 @@ def _model_to_summary(model: ModelRef, ledger: Ledger) -> ModelSummary: def _summarize(models: list[ModelRef], ledger: Ledger) -> list[ModelSummary]: """Build enriched ModelSummary rows via one batched backend dispatch.""" - backend = ledger._backend + backend = ledger.backend model_hashes = [m.model_hash for m in models] if hasattr(backend, "model_summaries"): enrichment = backend.model_summaries(model_hashes) @@ -84,7 +84,7 @@ def query(input: QueryInput, ledger: Ledger) -> QueryOutput: count_filters = dict(filters) if input.text: count_filters["text"] = input.text - backend = ledger._backend + backend = ledger.backend if hasattr(backend, "count_models"): total = backend.count_models(**count_filters) else: @@ -119,7 +119,7 @@ def _query_by_platform(input: QueryInput, ledger: Ledger, filters: dict[str, str backends) — so filtering and pagination happen here rather than in ``list_models``. """ - backend = ledger._backend + backend = ledger.backend models_all = ledger.list(**filters) if input.text: diff --git a/src/model_ledger/tools/record.py b/src/model_ledger/tools/record.py index 7632e7e..730eea8 100644 --- a/src/model_ledger/tools/record.py +++ b/src/model_ledger/tools/record.py @@ -20,7 +20,7 @@ def record(input: RecordInput, ledger: Ledger) -> RecordOutput: is not ``"registered"``. """ if input.event == "registered": - existing = ledger._backend.get_model_by_name(input.model_name) + existing = ledger.backend.get_model_by_name(input.model_name) if existing is None: model = ledger.register( name=input.model_name, @@ -32,7 +32,7 @@ def record(input: RecordInput, ledger: Ledger) -> RecordOutput: payload=input.payload or None, ) # register() appended the single canonical "registered" snapshot. - snapshot = ledger._backend.latest_snapshot(model.model_hash) + snapshot = ledger.backend.latest_snapshot(model.model_hash) if snapshot is None: # pragma: no cover — register always logs one raise RuntimeError("register() did not log a registration event") return RecordOutput( @@ -45,7 +45,7 @@ def record(input: RecordInput, ledger: Ledger) -> RecordOutput: # Idempotent re-registration: point at the existing registration # event instead of appending a duplicate. - registered = ledger._backend.list_snapshots(existing.model_hash, event_type="registered") + registered = ledger.backend.list_snapshots(existing.model_hash, event_type="registered") if registered: snapshot = max(registered, key=lambda s: s.timestamp) else: diff --git a/src/model_ledger/tools/schemas.py b/src/model_ledger/tools/schemas.py index 4b7d802..f18ce1a 100644 --- a/src/model_ledger/tools/schemas.py +++ b/src/model_ledger/tools/schemas.py @@ -9,7 +9,7 @@ from datetime import datetime from typing import Any, Literal -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, field_validator # --------------------------------------------------------------------------- # Shared types @@ -70,6 +70,14 @@ class RecordInput(BaseModel): purpose: str | None = None tier: str | None = None + @field_validator("model_name") + @classmethod + def _model_name_not_blank(cls, v: str) -> str: + """Whitespace-only names would register near-invisible models.""" + if not v.strip(): + raise ValueError("model_name must contain non-whitespace characters") + return v + class RecordOutput(BaseModel): """Output from the record tool. diff --git a/src/model_ledger/tools/tag.py b/src/model_ledger/tools/tag.py index 670858f..c38708e 100644 --- a/src/model_ledger/tools/tag.py +++ b/src/model_ledger/tools/tag.py @@ -21,7 +21,7 @@ def tag(input: TagInput, ledger: Ledger) -> TagOutput: def list_tags(model_name: str, ledger: Ledger) -> TagListOutput: """Return all tags attached to a model.""" ref = ledger.get(model_name) - tags = ledger._backend.list_tags(ref.model_hash) + tags = ledger.backend.list_tags(ref.model_hash) return TagListOutput( model_name=model_name, tags=[_tag_to_output(model_name, t) for t in tags], diff --git a/src/model_ledger/tools/trace.py b/src/model_ledger/tools/trace.py index ebf0bde..9b541cb 100644 --- a/src/model_ledger/tools/trace.py +++ b/src/model_ledger/tools/trace.py @@ -59,7 +59,7 @@ def trace(input: TraceInput, ledger: Ledger) -> TraceOutput: ModelNotFoundError: If the target model does not exist. """ ledger.get(input.name) - backend = ledger._backend + backend = ledger.backend upstream_levels: list[tuple[str, int]] = [] if input.direction in ("upstream", "both"): @@ -95,7 +95,9 @@ def _node(name: str, depth: int, relationship: str) -> DependencyNode: upstream_nodes = [_node(n, d, "depends_on") for n, d in upstream_levels] downstream_nodes = [_node(n, d, "feeds_into") for n, d in downstream_levels] - total = len(upstream_nodes) + len(downstream_nodes) + # Distinct models: with direction="both" (e.g. in a cycle) a node can be + # reachable in both directions but must be counted once. + total = len({n for n, _ in upstream_levels} | {n for n, _ in downstream_levels}) return TraceOutput( root=input.name, upstream=upstream_nodes, diff --git a/tests/test_backends/test_http_ledger_integration.py b/tests/test_backends/test_http_ledger_integration.py index df0c0cb..bcf7521 100644 --- a/tests/test_backends/test_http_ledger_integration.py +++ b/tests/test_backends/test_http_ledger_integration.py @@ -367,3 +367,11 @@ def test_record_empty_model_name_registered_is_rejected(self, server_client): ) assert resp.status_code == 422 assert server_client.get("/query").json()["total"] == 0 + + def test_record_whitespace_only_model_name_is_rejected(self, server_client): + resp = server_client.post( + "/record", + json={"model_name": " ", "event": "registered", "actor": "old-client"}, + ) + assert resp.status_code == 422 + assert server_client.get("/query").json()["total"] == 0 diff --git a/tests/test_tools/test_trace.py b/tests/test_tools/test_trace.py index e2342f3..1ddf00d 100644 --- a/tests/test_tools/test_trace.py +++ b/tests/test_tools/test_trace.py @@ -379,3 +379,30 @@ def counting_list_snapshots(*args, **kwargs): backend.list_snapshots = original assert bounded < unbounded + + +class TestTotalNodesCountsDistinctModels: + def test_cycle_traced_both_ways_counts_each_model_once(self, ledger): + for name in ("a", "b"): + ledger.register( + name=name, owner="risk-team", model_type="ml_model", tier="low", purpose="fixture" + ) + ledger.link_dependency("a", "b", actor="graph_builder") + ledger.link_dependency("b", "a", actor="graph_builder") + + out = trace(TraceInput(name="a", direction="both"), ledger) + # b is reachable upstream AND downstream of a — one distinct model. + assert len(out.upstream) == 1 + assert len(out.downstream) == 1 + assert out.total_nodes == 1 + + def test_disjoint_directions_still_sum(self, ledger): + for name in ("mid", "up", "down"): + ledger.register( + name=name, owner="risk-team", model_type="ml_model", tier="low", purpose="fixture" + ) + ledger.link_dependency("up", "mid", actor="graph_builder") + ledger.link_dependency("mid", "down", actor="graph_builder") + + out = trace(TraceInput(name="mid", direction="both"), ledger) + assert out.total_nodes == 2 From b2e058a7c21ead3889e5eccc9d2abf4a2ee4538f Mon Sep 17 00:00:00 2001 From: Vignesh Narayanaswamy Date: Sun, 19 Jul 2026 17:51:54 -0700 Subject: [PATCH 22/22] docs: correct HTTP snapshot-identity claims; CHANGELOG for the fix round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The get_snapshot comment implied rebuilt snapshots always recompute the identity the server holds; that is true only for server-minted events. Snapshots created client-side by Ledger.record() carry a client timestamp, so their hashes are process-local (cache-resolvable only) — say so explicitly. CHANGELOG: fold the review fix round into the v0.7.13 section (full changelog history over HTTP, registration fidelity, CTE/DELETE exclusions, sqlite3.Row, duck-typed backend validation, whitespace-name rejection, distinct total_nodes, stub-module dispatch coverage), add a compatibility note deferring the excel-extra removal and strict backend validation to 0.8.0, and drop the claims the review refuted. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 21 +++++++++++++-------- src/model_ledger/backends/http.py | 11 +++++++---- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d5706f..14fdc82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,18 +2,23 @@ ## v0.7.13 (Unreleased) -- fix: `Ledger(HttpLedgerBackend(url))` no longer corrupts the remote ledger. `append_snapshot` sends the real model name instead of `""` (which auto-registered a phantom empty-name model and attached every recorded event to it) and raises `ModelNotFoundError` for unresolvable hashes; registration logs exactly one `registered` event instead of double-firing (re-registration is now idempotent); `get_snapshot()` serves appended snapshots from a client-side cache and reconstructs older ones from `GET /changelog` instead of always returning `None`. Defense in depth: `RecordInput.model_name` requires `min_length=1`, so old clients that POST an empty model name get a 422 instead of silently corrupting the inventory. A new integration suite drives the SDK against the real `create_app()` handlers (not mocks). +Compatibility notes: this is a patch release — no public surface is removed. The `excel` extra is deprecated (unused since the scanner removal) and the strict full-protocol backend validation considered for this release was softened to a core-method duck check; both removals/tightenings are deferred to 0.8.0. + +- fix: `Ledger(HttpLedgerBackend(url))` no longer corrupts the remote ledger. `append_snapshot` sends the real model name instead of `""` (which auto-registered a phantom empty-name model and attached every recorded event to it) and raises `ModelNotFoundError` for unresolvable hashes; registration logs exactly one `registered` event instead of double-firing (re-registration is now idempotent); `get_snapshot()` serves appended snapshots from a client-side cache and reconstructs older ones from `GET /changelog` instead of always returning `None` (hashes rebuilt from the changelog match server-minted identities; hashes of snapshots created client-side by `Ledger.record()` are process-local and resolve via the cache only). Defense in depth: `RecordInput.model_name` requires `min_length=1` and rejects whitespace-only names, so old clients that POST an empty model name get a 422 instead of silently corrupting the inventory. A new integration suite drives the SDK against the real `create_app()` handlers (not mocks). +- fix: `HttpLedgerBackend.list_snapshots` fetches the model's full history. It called `GET /changelog` without a `since` bound, inheriting the changelog tool's 7-day default — so for any model whose events were older than a week, `get_snapshot` reconstruction returned `None`, `latest_snapshot` returned `None` (making `Ledger.tag` raise on healthy old models), re-registration lost its idempotency check, and the `platform` query filter returned zero results. It now passes an explicit all-time `since`. `list_models` also caches name mappings for the hashes it hands out, so they stay resolvable within the process (`batch_platforms`, and with it the platform filter, works from a fresh client). +- fix: registration over `HttpLedgerBackend` carries the caller's payload, tier, and actor to the server. `Ledger.register()` dispatches to an optional backend `register_model` hook (implemented by the HTTP backend as a single `POST /record`); previously the payload was silently dropped, and every remote registration landed as `tier="unclassified"`, `actor="user"`. `RecordInput` gains an optional `tier` field (additive). - fix: the `query` tool's `platform` filter is actually applied. It was advertised in `QueryInput` and the REST/MCP schemas but never read — any platform value returned the full inventory. Platform is resolved from snapshot data in one batched `batch_platforms` dispatch and filtered/paginated in the tool; a schema-vs-implementation parity test now guards every `QueryInput` filter field. - fix: the introspector registry dispatches XGBoost sklearn-API models to the xgboost introspector. `SklearnIntrospector.can_handle` now rejects estimators from wrapper frameworks with dedicated introspectors (xgboost, lightgbm) instead of claiming them via `BaseEstimator`, making dispatch independent of entry-point ordering. -- fix: SQL lineage extraction accepts unqualified (bare) table names — `INSERT INTO feature_store SELECT * FROM raw_events` previously parsed to zero reads and zero writes, silently producing no lineage. A keyword blocklist keeps subqueries, table functions, and row sources out of the results; `INSERT OVERWRITE` is recognized. -- fix: the `trace` tool reports real BFS depth instead of fabricating it from list position. `depth` was computed from each node's index in the flat transitive list, so any model with N transitive upstreams rendered as an N-deep linear chain (a rich 8-level fan-out graph showed as "43 nodes at depths 1–43"); downstream had the same bug. Traversal is now a breadth-first walk that records each node's actual level (shortest edge distance, one entry per node at its minimum depth) and terminates at `input.depth` — bounded queries now do bounded work instead of filtering after a full traversal. +- fix: SQL lineage extraction accepts unqualified (bare) table names — `INSERT INTO feature_store SELECT * FROM raw_events` previously parsed to zero reads and zero writes, silently producing no lineage. CTE names (`WITH ... AS`, including nested and `RECURSIVE` forms) and `DELETE FROM` targets are excluded from read tables, so dbt-style SQL does not produce phantom lineage inputs; a keyword blocklist keeps subqueries, table functions, and row sources out of the results (a bare table genuinely named a filtered keyword must be schema-qualified to extract); `INSERT OVERWRITE` is recognized, including the Hive/Spark `INSERT OVERWRITE TABLE t` form. +- fix: the `trace` tool reports real BFS depth instead of fabricating it from list position. `depth` was computed from each node's index in the flat transitive list, so any model with N transitive upstreams rendered as an N-deep linear chain (a rich 8-level fan-out graph showed as "43 nodes at depths 1–43"); downstream had the same bug. Traversal is now a breadth-first walk that records each node's actual level (shortest edge distance, one entry per node at its minimum depth) and terminates at `input.depth` — bounded queries now do bounded work instead of filtering after a full traversal. `total_nodes` counts distinct models (a node reachable both upstream and downstream with `direction="both"` was counted twice). - fix: `strip_template_vars` strips spaced template variables like `{{ ds }}`; its docstring example now shows the actual output. -- fix: `sql_connector` raises a clear `TypeError` naming the dict-row requirement when the connection yields tuple rows, instead of an opaque indexing error. The connectors guide documents the requirement with a sqlite3 `row_factory` example. -- fix: `create_app()` / `create_server()` validate the backend at construction and raise a clear `TypeError` (with hints for the common Ledger-instead-of-backend and path-string mistakes) instead of failing deep inside the first request with `AttributeError`. -- fix(cli): `export --output` help text now says "Output file path" (the command writes a single file, not a directory), the default output gained a proper `.html` extension, and the success message prints the real artifact path. -- feat: `Ledger.register()` accepts an optional `payload` dict merged into the registration event's payload (caller keys win) — used by the record tool so the single `registered` event still carries the caller's payload. -- chore: remove the vestigial `excel` extra — nothing has imported openpyxl since the scanner module was removed post-v0.7.7. +- fix: `sql_connector` raises a clear `TypeError` naming the dict-row requirement when the connection yields tuple rows, instead of an opaque indexing error; name-addressable rows without the full dict API (e.g. `sqlite3.Row`, which has `keys()` but no `.get()`) are accepted. The connectors guide documents the requirement with a `row_factory = sqlite3.Row` example. +- fix: `create_app()` / `create_server()` validate the backend at construction and raise a clear `TypeError` (with hints for the common Ledger-instead-of-backend and path-string mistakes) instead of failing deep inside the first request with `AttributeError`. Validation is a core-method duck check, not full-protocol `isinstance`, so partial custom backends that worked on 0.7.12 keep working. +- fix(cli): `export --output` help text now says "Output file path" (the command writes a single file, not a directory) and the success message prints the real artifact path. The default output filename is unchanged (`audit_pack`). +- feat: `Ledger.register()` accepts an optional `payload` dict merged into the registration event's payload (caller keys win) — used by the record tool so the single `registered` event still carries the caller's payload, over local and HTTP backends alike. `Ledger` also gains a public read-only `backend` property. +- chore: deprecate the `excel` extra — nothing has imported openpyxl since the scanner module was removed post-v0.7.7. The extra is kept so `model-ledger[excel]` pins keep resolving within 0.7.x; removal planned for 0.8.0. - test: pandas-dependent tests skip instead of hard-failing when pandas is absent alongside the ML introspection libs. +- test: introspector dispatch tests execute in every environment via stub sklearn/xgboost/lightgbm modules (the real-library variants still run where the ML extras are installed) — previously all dispatch tests skipped in CI. ## v0.7.12 diff --git a/src/model_ledger/backends/http.py b/src/model_ledger/backends/http.py index 500937f..0e91d80 100644 --- a/src/model_ledger/backends/http.py +++ b/src/model_ledger/backends/http.py @@ -197,10 +197,13 @@ def get_snapshot(self, snapshot_hash: str) -> Snapshot | None: cached = self._snapshot_cache.get(snapshot_hash) if cached is not None: return cached - # The REST API has no snapshot-by-hash endpoint. Snapshot hashes are - # content-derived (model_hash + timestamp + payload), so snapshots - # rebuilt from GET /changelog recompute the same identity the server - # holds — scan the changelog of every model resolved so far. + # The REST API has no snapshot-by-hash endpoint, so scan the + # changelog of every model resolved so far. Snapshot hashes are + # content-derived (model_hash + timestamp + payload), which means + # hashes rebuilt here match SERVER-minted identities. Snapshots + # created client-side by Ledger.record() carry a client timestamp, + # so their hashes are process-local: resolvable through this + # backend's _snapshot_cache, but not from another client. for model_hash in list(self._hash_to_name): for snap in self.list_snapshots(model_hash): if snap.snapshot_hash == snapshot_hash: