fix: SQL fallback snapshot flush drops PAYLOAD and TAGS - #33
Merged
Conversation
_flush_snapshots_sql — the DDL-free path taken whenever the role cannot CREATE TEMPORARY TABLE (least-privilege deployments: INSERT/UPDATE only) — inserted only the 7 scalar columns, silently persisting every snapshot with NULL PAYLOAD and TAGS. The pandas path carries both; the fallback must too. Payload/tags now ride the UNION ALL source as escaped JSON strings and land via PARSE_JSON in the INSERT...SELECT (expressions are not allowed in VALUES, but are in SELECT). Two regression tests: content preservation, and NULL (not 'null') for empty payloads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4fc2113c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+433
to
+434
| f"{_esc(json.dumps(s.payload, default=str)) if s.payload else 'NULL'}, " | ||
| f"{_esc(json.dumps(s.tags, default=str)) if s.tags else 'NULL'}" |
There was a problem hiding this comment.
When the SQL fallback is used and a payload/tag string contains JSON escapes (\n, \t, \\, \uXXXX, Windows paths, regexes), _esc(json.dumps(...)) only doubles quotes but leaves backslashes inside a Snowflake single-quoted literal. Snowflake treats backslashes in such literals as escape sequences (docs), so PARSE_JSON sees different or invalid JSON (for example, an actual newline inside a JSON string) and the flush can fail or corrupt payloads; escape backslashes or use dollar-quoted/parameterized literals before parsing.
Useful? React with 👍 / 👎.
vigneshnarayanaswamy
added a commit
that referenced
this pull request
Jul 10, 2026
…35) Blind post-merge review of #33/#34 found the composition bug: Snowflake processes backslash escape sequences inside single-quoted constants, and _esc only doubled single quotes — so any JSON payload containing an embedded double quote (serialized by json.dumps as \") or a backslash reached PARSE_JSON mangled. On the SQL fallback path (which #34 routes all non-native-transport deployments onto) that meant: INSERT fails, buffer never clears, every subsequent flush-before-read 500s — the poisoned-buffer incident, reintroduced on common data. Verified live: PARSE_JSON('{"a": "x \" y"}') errors; the doubled form round-trips. Also from the review: privilege denials (the expected steady state of least-privilege roles) short-circuit quietly again instead of warning + issuing a doomed DROP per flush; unexpected failures log with exc_info; the failure-path cleanup DROP is gone (temp tables die with the session, and it could target a same-named permanent table); row construction sits inside the try; the null-payload regression test inspected the wrong side of FROM and could never fail — fixed; snapshot-path fallback and quote/backslash round-trip tests added. Version 0.7.12. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
vigneshnarayanaswamy
added a commit
that referenced
this pull request
Jul 21, 2026
…ter, trace depth, SQL lineage extraction) (#36) * fix: HttpLedgerBackend routed events to a phantom empty-name model 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 <noreply@anthropic.com> * fix: query silently ignored the platform filter 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix: SQL lineage extraction silently dropped unqualified table names 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * test: skip, don't fail, pandas-dependent tests when pandas is absent 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix: create_app/create_server reject non-backend objects at construction 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix(sql): CTE names and DELETE FROM targets are not read tables; handle INSERT OVERWRITE TABLE 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 <noreply@anthropic.com> * fix(http): list_snapshots fetches the full history, not the changelog's 7-day default 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 <noreply@anthropic.com> * fix(http): registration payload, tier, and actor reach the server 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix(backends): validate_backend duck-checks core methods instead of full-protocol isinstance 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 <noreply@anthropic.com> * test(introspect): dispatch tests execute without the ML libraries 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 <noreply@anthropic.com> * revert: keep 0.7.13 patch-safe — restore excel extra and export default filename 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 <noreply@anthropic.com> * fix(tools): whitespace-only names rejected; trace total_nodes counts distinct models; public Ledger.backend 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 <noreply@anthropic.com> * docs: correct HTTP snapshot-identity claims; CHANGELOG for the fix round 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The DDL-free SQL fallback (
_flush_snapshots_sql) — the path taken whenever the role can'tCREATE TEMPORARY TABLE, i.e. every least-privilege deployment with row-level grants only — inserted just the 7 scalar columns. Every snapshot flushed through it persisted with NULLPAYLOADandTAGS, silently dropping the event data itself (discovered ports/metadata, observation payloads, record bodies).The pandas path has always carried both columns; the fallback now matches it: payload/tags ride the
UNION ALLsource as escaped JSON strings and land viaPARSE_JSON(...)in theINSERT...SELECT(expressions are not allowed inVALUES, but are inSELECT).Regression tests: content preservation through the fallback, and SQL
NULL(not the string'null') for empty payload/tags.Found by automated review on the downstream repin (a deployment whose writer role has exactly this grant shape); verified against the wheel source before fixing.
🤖 Generated with Claude Code