From 5c50e6bef47fe82138bef3978f83d7df60b06df9 Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 22 Jul 2026 15:58:30 +0400 Subject: [PATCH 1/5] Reject case-colliding ids across all storage backends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit YAMLStorage uses raw ids as filenames, so ids differing only by case (X vs x) address the same file on case-insensitive filesystems (macOS, Windows) and silently overwrite each other — datasources, models, and memories alike. SQLiteStorage kept them distinct, so the backends silently diverged. Saving an id that differs only by case from an existing one now raises IdCollisionError(SlayerError, ValueError), enforced in the StorageBackend base templates so every backend rejects on every platform and stores stay portable. Exact-id re-saves remain upserts; a legacy store already holding a colliding pair raises on a save of either spelling. - save_datasource becomes a template method delegating to a new abstract _save_datasource_impl (same pattern as save_model) - save_memory checks via a new cheap _list_memory_ids() primitive; SQLiteStorage's atomic save_memory override duplicates the check inside its transaction - model saves check both identity components: data_source against datasource names and other models' data_sources, name within the same datasource - YAML reads/deletes verify the exact directory entry via os.listdir comparison, so a case-variant lookup reads as not-found and a delete is a no-op instead of hitting the wrong file - migrate_memories_layout and migrate_yaml_layout pre-check collisions before writing anything, preserving legacy files on failure - surfacing: REST POST /datasources maps to 400; MCP create_datasource returns a friendly message and isolates per-model ingest-save failures; CLI prints and exits or skips-and-continues in ingest loops Closes #249 Claude-Session: https://claude.ai/code/session_011sbQKNXcUtGAsCXtmJXHvR --- CLAUDE.md | 1 + docs/concepts/ingestion.md | 2 + docs/concepts/memories.md | 6 +- docs/concepts/models.md | 2 +- docs/configuration/storage.md | 17 +- slayer/api/server.py | 10 +- slayer/cli.py | 18 +- slayer/core/errors.py | 41 ++++ slayer/mcp/server.py | 26 ++- slayer/storage/base.py | 111 ++++++++++- slayer/storage/join_sync.py | 13 ++ slayer/storage/sqlite_storage.py | 26 ++- slayer/storage/v4_migration.py | 51 +++++ slayer/storage/yaml_storage.py | 83 ++++++-- tests/test_api_server.py | 11 ++ tests/test_id_collision.py | 326 +++++++++++++++++++++++++++++++ tests/test_memory_string_ids.py | 18 +- tests/test_yaml_exact_entry.py | 127 ++++++++++++ 18 files changed, 839 insertions(+), 50 deletions(-) create mode 100644 tests/test_id_collision.py create mode 100644 tests/test_yaml_exact_entry.py diff --git a/CLAUDE.md b/CLAUDE.md index 75511500..13d93d18 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,6 +53,7 @@ poetry run ruff check slayer/ tests/ - **Schema versioning**: `SlayerModel`, `SlayerQuery`, and `DatasourceConfig` carry a `version: int` (currently `7` for `SlayerModel`, `3` for `SlayerQuery`, `1` for `DatasourceConfig`). On load, older versions are upgraded via the converter chain in `slayer/storage/migrations.py` before Pydantic validates the dict. Saves always emit the current version. The hook is on the Pydantic class itself (`@model_validator(mode="before")`), so every storage backend — YAML, SQLite, third-party backends, plus MCP/API/dbt entry points — gets migrations automatically without backend changes. The v1→v2 converter (in `slayer/storage/v2_migration.py`) merges v1 `dimensions`+`measures` into v2 `columns`, repurposes `measures` to hold `ModelMeasure` formulas, and renames `SlayerQuery.fields`→`measures`. The v2→v3 converter (in `slayer/storage/v3_migration.py`) drops the legacy `dry_run`/`explain` fields from `SlayerQuery` (they are now engine kwargs only — `engine.execute(query, dry_run=..., explain=...)`) and walks `SlayerModel.source_queries` entries through the SlayerQuery chain. The v3→v4 converter (in `slayer/storage/v4_migration.py`) requires non-empty `data_source` on table-backed SlayerModel dicts (`sql_table` or `sql` mode); query-backed models (`source_queries` set) are exempt because their `data_source` is filled by `engine._validate_and_populate_cache` from the resolved virtual model before save. The v4 converter also ships layout migrators that move legacy `models/.yaml` flat files into `models//.yaml` and rebuild the SQLite `models` table with a composite `(data_source, name)` primary key. The v4→v5 converter (in `slayer/storage/v5_migration.py`, DEV-1361) coarse-renames `Column.type` legacy values to the new sqlglot-aligned vocabulary (`string→TEXT`, `number→DOUBLE`, `time→TIMESTAMP`, etc.) and strips the dead aggregation pseudo-types (`count`/`sum`/...). The v5→v6 converter (in `slayer/storage/v6_migration.py`, DEV-1375) is a no-op forward — v6 introduces a single new optional field, `Column.sampled: Optional[str]`, that caches the per-column sample-value snapshot consumed by [`search`](docs/concepts/search.md) and `inspect_model`. The v6→v7 converter (in `slayer/storage/v7_migration.py`, DEV-1480) is also a no-op forward — v7 adds two more `Column` fields: `sampled_values: Optional[List[str]]` (the structured top-50-by-frequency list, paired with the existing text `sampled`) and `distinct_count: Optional[int]` (true cardinality at profile time, surfaced via a secondary `count_distinct` query on overflow). Storage backends additionally introspect each model's datasource on first load and refine `DOUBLE → INT` for base columns whose live SQL type is integer (`slayer/storage/type_refinement.py`); the refined model is written back so subsequent loads skip both steps. `SlayerQuery` v3 sets `extra="forbid"` so unknown fields raise. See [docs/concepts/models.md](docs/concepts/models.md#schema-versioning). - **DataType / CAST emission** (v5, DEV-1361): `DataType` values match sqlglot's `exp.DataType.Type` byte-for-byte: `TEXT`, `INT`, `DOUBLE`, `BOOLEAN`, `DATE`, `TIMESTAMP`. Auto-ingestion narrows integer DB types (INTEGER/BIGINT/SERIAL/INT8…/UINT8…) to `INT`, and floats/numerics (FLOAT/DOUBLE/DECIMAL with scale>0) to `DOUBLE`; NUMERIC(p,0) and DECIMAL(p,0) are integer-shaped and narrow to `INT`. The SQL generator wraps **non-bare** `Column.sql` (function calls, arithmetic, CASE WHEN) in `CAST(... AS )` driven by `Column.type`; bare identifiers and `sql=None` are emitted unchanged. `TEXT` is a no-op (skipped, since `CAST AS TEXT` is cosmetic and doesn't unwrap SQLite's JSON-quoted strings anyway). `ModelMeasure.type` (also `Optional[DataType]`) declares the formula's result type; when set, the aggregation expression is wrapped in an outer CAST. Lenient `before`-validators on `Column.type` and `ModelMeasure.type` absorb legacy lowercase agent input (`"string"` → `TEXT`, `"number"` → `DOUBLE`, etc.) and silently drop dropped pseudo-types. `slayer storage migrate-types --dry-run [--data-source X]` runs the storage refinement step explicitly for batch / inspectable usage. Schema-drift detection (`data_type_bucket`) keeps `INT` and `DOUBLE` in the same `"number"` bucket so a `DOUBLE`-typed persisted column does not flag as drift against an `INT` live column. - **Datasource-scoped storage** (v4, DEV-1330): Models are keyed by `(data_source, name)`, not bare `name`. Two datasources can share a table name without collision. `storage.get_model(name, data_source=None)` and `storage.delete_model(name, data_source=None)` resolve bare names by: (1) returning the unique match if exactly one model has that name, (2) walking `storage.get_datasource_priority()` to pick the first datasource in the priority list that has the name, (3) raising `slayer.core.errors.AmbiguousModelError` otherwise. The priority list is configured via `storage.set_datasource_priority(["db_a", ...])`, the MCP `set_datasource_priority` tool, the REST `PUT /datasources/priority`, or the `slayer datasources priority` CLI subcommand. `engine.execute(query, data_source=...)` passes a hint that wins over the priority list. Joins resolve targets within the parent model's `data_source` only — cross-datasource joins are not auto-mirrored. A sibling Linear issue ([DEV-1342](https://linear.app/motley-ai/issue/DEV-1342)) tracks whether to add `datasource.model_name` dot syntax inside query strings. +- **Case-collision rejection** (issue #249): saving a datasource, model, or memory whose id differs only by letter case (`casefold()`-equal, raw-different) from an existing one raises `slayer.core.errors.IdCollisionError(SlayerError, ValueError)` — attrs `kind`/`new_id`/`existing_id`/`data_source`. Ids become filenames in the YAML backend (case variants alias on macOS/Windows); the check is enforced in the `StorageBackend` base templates so EVERY backend rejects on EVERY platform and stores stay portable. Exact-id re-saves remain upserts; a legacy store already holding a colliding pair raises on a save of either spelling. Model saves check both components: `data_source` against registered datasource names ∪ other models' data_sources (shared `models//` dir namespace), and `name` within the same datasource. `save_datasource` is now a base template method delegating to abstract `_save_datasource_impl` (backends implement only the impl — same pattern as `save_model`/`_save_model_impl`); `save_memory` checks via the cheap `_list_memory_ids()` primitive (YAML: listdir; SQLite: `SELECT id`; JoinSync: delegation). `SQLiteStorage.save_memory` bypasses the base template, so the same check is duplicated inside `_save_memory_atomic_sync`'s transaction. `save_model(_validate=False)` (migration write-back) bypasses the check. YAML reads/deletes verify the EXACT directory entry via `os.listdir` comparison (`_exact_entry_exists`) — a case-variant `get_model`/`get_datasource`/`get_memory` returns not-found and a delete is a no-op instead of silently hitting the wrong file. `migrate_memories_layout` and `migrate_yaml_layout` pre-check collisions before writing anything (legacy files preserved on failure). Surfacing: REST `POST /datasources` → 400 (model/memory endpoints already mapped ValueError→400); MCP `create_datasource` returns a friendly message and its auto-ingest loop reports per-model save failures without aborting; CLI prints and exits (or skips-and-continues in ingest loops). - **Source modes**: a `SlayerModel` has exactly one source: `sql_table` (physical table), `sql` (explicit SQL subquery), or `source_queries` (query-backed: `List[SlayerQuery]`). Validators enforce mutual exclusivity, reject empty `source_queries=[]`, require `name` on every non-final stage, and reject duplicate stage names. `SlayerModel.source_queries` entries given as dicts are auto-parsed into `SlayerQuery` instances by a Pydantic before-validator. - **Stages form a DAG, not just a chain**: any stage in `source_queries` or a runtime query list may use a named sibling as `source_model` or as `joins.target_model`. The **runtime list path** (`engine.execute(query=[...])`, CLI `slayer query @file.json`, MCP `query_nested`) **auto-sorts** the input via `SlayerQueryEngine._topologically_order_queries` — a hand-rolled Kahn's algorithm (no `graphlib` dep) that reorders so every stage appears after the siblings it references. The last entry of the input stays last as the entry point / DAG root, so the result returned doesn't depend on sort order. Validations performed up front: missing `name` on a non-final entry; duplicate names; self-references; the root being referenced by any other stage (root must be the dependency sink); cycles. Unreachable utility sub-queries are accepted — they flow through the sort but are silently dropped from the emitted SQL since nothing resolves them. Stored `SlayerModel.source_queries` retains strict-order semantics (relies on `_scope_named_queries_to_prior` plus the `_forbidden_sibling_refs_var` ContextVar in `_resolve_model_inner` / `_resolve_join_target`) — YAML-defined source_queries are read top-to-bottom and the strict-order error pins typos at resolve time. The runtime list's reference extractor walks `source_model` (string sibling refs), `ModelExtension.source_name`, and `joins[].target_model` from both dict and typed shapes; formula-only references resolve correctly because the explicit join they require is what the extractor picks up. - **Query-backed models**: `SlayerModel.query_variables: Dict[str, Any]` provides defaults for `{var}` placeholders in `source_queries`. `engine.create_model_from_query(query, name, variables=..., save=True)` saves a query as a query-backed model and populates the `columns` + `backing_query_sql` cache. `engine.save_model(model)` is the engine-side save helper that runs source-mode validation + cache refresh + persistence; user-supplied `columns` and `backing_query_sql` on a query-backed model are rejected at save with a clear error. The cache is refreshed only on save paths (`engine.save_model` / `create_model_from_query(save=True)`); `engine.execute` never writes to storage, even on stale or empty caches (closes #74 — `tests/test_query_backed_models.py::test_execute_never_writes_to_storage` pins this). diff --git a/docs/concepts/ingestion.md b/docs/concepts/ingestion.md index 934d59aa..2d2e6bbd 100644 --- a/docs/concepts/ingestion.md +++ b/docs/concepts/ingestion.md @@ -276,6 +276,8 @@ Ingest-on-startup: N/M datasources ingested (K failed: name1, name2) - **Existing `sql_table`-mode model** → append new columns and joins from the live schema. Existing columns and joins are **never** mutated — `description`, `label`, `format`, `meta`, and `allowed_aggregations` are preserved verbatim. - **Existing `sql`-mode or query-backed model with the matching name** → skipped silently; those are user-authored. +Two live tables whose quoted names differ only by letter case (`"Orders"` vs `orders`) cannot both be persisted — model names collide as filenames in YAML storage on macOS / Windows, so the save is rejected uniformly (`IdCollisionError`). The first table wins; the second surfaces as a per-model entry in `IdempotentIngestResult.errors` (or a per-model message on the CLI / MCP paths) without aborting the rest of the ingest. + After the additive pass, `validate_models` runs against the in-scope models and the result is merged into the response (`IdempotentIngestResult.to_delete`). Type-bucket drift on existing columns surfaces there — apply via `slayer validate-models --force-clean`, then re-ingest to pick up the new live type. See [Schema Drift](schema-drift.md) for the full diff / cascade contract. ### Search side effects diff --git a/docs/concepts/memories.md b/docs/concepts/memories.md index 23a6edd1..34674ef3 100644 --- a/docs/concepts/memories.md +++ b/docs/concepts/memories.md @@ -77,7 +77,11 @@ int-shaped id (`"1"`, `"2"`, ...). Supply a string for a stable user-controlled id (`"kb.policy.42"`) — useful for knowledge-base ingestion pipelines. Charset excludes `:`, `/`, `?`, `#`, whitespace, and ASCII control characters. Duplicate id → unconditional **upsert**, -`created_at` preserved. +`created_at` preserved. An id that differs only by letter case from an +existing one (`X` vs `x`) raises `IdCollisionError` — ids become +filenames in the YAML backend, where case variants collide on +macOS / Windows, and the rule is enforced on every backend so stores +stay portable. `description` is optional (≤ 500 chars). When set, `search(compact=True)` and `inspect_model(compact=True)` surface this short preview instead of diff --git a/docs/concepts/models.md b/docs/concepts/models.md index b6e662d8..81b135f6 100644 --- a/docs/concepts/models.md +++ b/docs/concepts/models.md @@ -22,7 +22,7 @@ A query then asks for `revenue:sum` (aggregate the `revenue` column), `aov` (the | Field | Type | Required | Description | |-------|------|----------|-------------| -| `name` | string | Yes | Unique model name | +| `name` | string | Yes | Unique model name. Names differing only by letter case (`Orders` vs `orders`) are rejected at save time (`IdCollisionError`) — they would collide as filenames in YAML storage on macOS / Windows | | `sql_table` | string | One of | A physical database table (e.g. `public.orders`) | | `sql` | string | these | A SQL subquery to use as the source | | `source_queries` | list[SlayerQuery] | three | Saved query stages — makes the model **query-backed** | diff --git a/docs/configuration/storage.md b/docs/configuration/storage.md index 3d8e5739..391ff253 100644 --- a/docs/configuration/storage.md +++ b/docs/configuration/storage.md @@ -64,6 +64,8 @@ slayer_data/ **Layout note:** Models live under `models//.yaml` so two datasources sharing a table name don't collide. Opening a `YAMLStorage` on a legacy flat directory migrates `models/.yaml` files into the nested layout automatically. If a flat file has an empty `data_source` and exactly one datasource is registered, the migrator auto-fills it; otherwise it hard-fails so the user can edit `data_source` by hand before reopening. +**Name collisions:** Because names become filenames here, two ids differing only by letter case (`Orders` vs `orders`) would address the same file on macOS / Windows. Saving a datasource, model, or memory whose id differs only by case from an existing one therefore raises `IdCollisionError` — on **every** backend and platform, so a store created on Linux stays portable. Re-saving the exact same id is still a normal upsert. On the read side, YAML lookups compare the exact filename, so `get_model("Orders")` when only `orders.yaml` exists returns "not found" instead of the wrong model (and a delete is a no-op). The layout migrations apply the same check up front and refuse to run — legacy files untouched — if migrating would collide. + **Embeddings sidecar (DEV-1405):** Embedding rows used by the optional dense-search channel live in a SQLite file at `/embeddings.db`, **not** in `embeddings.yaml`. Embeddings are derived artifacts (regeneratable by `slayer ingest` / `--ingest-on-startup`), not user-authored config, so the diffable-in-git property that drives the YAML choice for models doesn't apply. A pre-DEV-1405 `embeddings.yaml` or `counters.yaml` is silently renamed to `.yaml.legacy` on first open and ignored thereafter; re-run `slayer ingest` to repopulate `embeddings.db`. The schema is identical to the `SQLiteStorage` embedding table — both backends delegate to a shared `SidecarEmbeddingStore` helper. **Memory id allocation (DEV-1658):** The next int-shaped memory id is derived by scanning the `memories/` directory of per-id `.md` files — `max(int-shaped id) + 1` — rather than a separate counter file. Non-int ids (e.g. `help.intro`, `kb.policy.42`) are ignored by the allocator. Ids of deleted memories may be reused by future saves; cascade-on-delete in `delete_memory` already removes the matching embedding row. @@ -80,6 +82,8 @@ storage = SQLiteStorage(db_path="./slayer.db") Tables: `models`, `datasources`, `settings` (for the datasource priority list), `memories` + `memory_entities` (indexed by canonical entity), and `embeddings` (cached embedding rows keyed by `(canonical_id, embedding_model_name)`). Memory ids are assigned by SQLite's `INTEGER PRIMARY KEY` rowid mechanism inside the save transaction — no separate counter table is needed. +SQLite keys are case-sensitive, but the case-collision rule above applies here too (enforced in the shared base class) so a SQLite store can always be exported to the YAML layout and moved across platforms. + ## Storage Resolution The `resolve_storage()` factory creates a backend from a path or URI: @@ -109,19 +113,22 @@ from slayer.storage.base import StorageBackend from slayer.core.models import SlayerModel, DatasourceConfig class MyCustomStorage(StorageBackend): - # Models are keyed by (data_source, name). - def save_model(self, model: SlayerModel) -> None: ... + # Models are keyed by (data_source, name). ``save_model`` and + # ``save_datasource`` are template methods on the base class (they run + # shared validation, e.g. case-collision rejection) — backends implement + # only the ``_impl`` writes. + def _save_model_impl(self, model: SlayerModel) -> None: ... def _list_all_model_identities(self) -> list[tuple[str, str]]: ... def get_model(self, name: str, data_source: str | None = None) -> SlayerModel | None: ... - def delete_model(self, name: str, data_source: str | None = None) -> bool: ... + def _delete_model_row(self, *, data_source: str, name: str) -> bool: ... # ``StorageBackend`` provides default implementations of ``list_models`` # and ``resolve_model_identity`` on top of ``_list_all_model_identities``. - def save_datasource(self, datasource: DatasourceConfig) -> None: ... + def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: ... def get_datasource(self, name: str) -> DatasourceConfig | None: ... def list_datasources(self) -> list[str]: ... - def delete_datasource(self, name: str) -> bool: ... + def _delete_datasource_row(self, name: str) -> bool: ... ``` Register it for URI-based resolution: diff --git a/slayer/api/server.py b/slayer/api/server.py index 8cf92b18..57769b8e 100644 --- a/slayer/api/server.py +++ b/slayer/api/server.py @@ -523,9 +523,15 @@ async def get_datasource(name: str) -> dict[str, Any]: data[secret_field] = "***" return data - @app.post("/datasources") + @app.post( + "/datasources", + responses={400: {"description": "Name conflicts with an existing datasource (differs only by case)."}}, + ) async def create_datasource(datasource: DatasourceConfig) -> dict[str, str]: - await storage.save_datasource(datasource) + try: + await storage.save_datasource(datasource) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) return {"status": "created", "name": datasource.name} @app.delete("/datasources/{name}") diff --git a/slayer/cli.py b/slayer/cli.py index 90ce18b2..db116992 100644 --- a/slayer/cli.py +++ b/slayer/cli.py @@ -1805,7 +1805,11 @@ def _persist_ingested_models(models, storage, *, assume_yes: bool, pre_save=None for model in models: if pre_save is not None: pre_save(model) - run_sync(storage.save_model(model)) + try: + run_sync(storage.save_model(model)) + except ValueError as e: + print(f"Skipped {model.name}: {e}") + continue print(f"Ingested: {model.name} ({len(model.columns)} columns, {len(model.measures)} measures)") @@ -1839,7 +1843,11 @@ def _run_datasources_create(args, storage): print("Aborted.") sys.exit(1) - run_sync(storage.save_datasource(ds)) + try: + run_sync(storage.save_datasource(ds)) + except ValueError as e: + print(f"Error: {e}") + sys.exit(1) print(f"Created datasource '{ds.name}' ({ds.type}).") if not args.ingest: @@ -1907,7 +1915,11 @@ def _run_datasources_create_demo(args, storage): # NOSONAR S3776 — linear dem print("Aborted.") sys.exit(1) - run_sync(storage.save_datasource(ds)) + try: + run_sync(storage.save_datasource(ds)) + except ValueError as e: + print(f"Error: {e}") + sys.exit(1) print(f"Created datasource '{ds.name}' (duckdb).") if not args.ingest: diff --git a/slayer/core/errors.py b/slayer/core/errors.py index 79601be8..ae54582e 100644 --- a/slayer/core/errors.py +++ b/slayer/core/errors.py @@ -116,6 +116,47 @@ def __init__(self, cycle: list[tuple[str, str]]) -> None: super().__init__(f"Circular column reference detected: {chain}") +class IdCollisionError(SlayerError, ValueError): + """Raised when saving an entity whose id differs from an existing id + only by letter case (``Orders`` vs ``orders``). + + Ids become filenames in the YAML backend, where case-variant ids + address the same file on case-insensitive filesystems (macOS, + Windows). The check runs uniformly on every backend so stores stay + portable across backends and platforms. + + ``kind`` is one of ``"model"`` / ``"datasource"`` / ``"memory"``; + ``data_source`` is set for model-name collisions. Multi-inherits + ``ValueError`` so existing ``except ValueError`` call sites continue + to work unchanged. + """ + + _LABELS = { + "model": "Model name", + "datasource": "Datasource name", + "memory": "Memory id", + } + + def __init__( + self, + *, + kind: str, + new_id: str, + existing_id: str, + data_source: str | None = None, + ) -> None: + self.kind = kind + self.new_id = new_id + self.existing_id = existing_id + self.data_source = data_source + label = self._LABELS.get(kind, "Id") + scope = f" in datasource '{data_source}'" if data_source else "" + super().__init__( + f"{label} '{new_id}' conflicts with existing '{existing_id}'" + f"{scope} (differs only by case). Rename or delete one." + ) + + class ForcedFilterError(SlayerError): """Raised by the session-policy forced-filter rewrite (DEV-1578). diff --git a/slayer/mcp/server.py b/slayer/mcp/server.py index 63d556ae..ea5c4d28 100644 --- a/slayer/mcp/server.py +++ b/slayer/mcp/server.py @@ -1343,7 +1343,10 @@ async def create_datasource( ) ds = DatasourceConfig.model_validate(data) existed = await storage.get_datasource(name) is not None - await storage.save_datasource(ds) + try: + await storage.save_datasource(ds) + except ValueError as exc: + return f"Cannot create datasource: {exc}" verb = "replaced" if existed else "created" ok, msg = _test_connection(ds) @@ -1364,21 +1367,34 @@ async def create_datasource( return "\n".join(lines) raise + save_errors: list[str] = [] + saved_models = [] for model in models: - await storage.save_model(model) - - if not models: + try: + await storage.save_model(model) + saved_models.append(model) + except ValueError as exc: + # e.g. quoted case-variant tables ("Orders" vs orders) — + # report and continue instead of aborting the whole ingest. + save_errors.append(f"- {model.name}: {exc}") + models = saved_models + + if not models and not save_errors: lines.append("No tables found to ingest.") schemas = _get_schemas(ds) if schemas: lines.append(f"Available schemas: {', '.join(schemas)}") - else: + elif models: lines.append(f"Ingested {len(models)} model(s):") for m in models: lines.append(f"- {m.name} ({len(m.columns)} columns, {len(m.measures)} measures)") lines.append("") lines.append("Use models_summary and inspect to explore, then query to fetch data.") + if save_errors: + lines.append(f"Failed to save {len(save_errors)} model(s):") + lines.extend(save_errors) + return "\n".join(lines) @mcp.tool() diff --git a/slayer/storage/base.py b/slayer/storage/base.py index c7cd16bc..bc244069 100644 --- a/slayer/storage/base.py +++ b/slayer/storage/base.py @@ -6,9 +6,13 @@ from abc import ABC, abstractmethod from pathlib import Path from typing import Any -from collections.abc import Callable +from collections.abc import Callable, Iterable -from slayer.core.errors import AmbiguousModelError, MemoryNotFoundError +from slayer.core.errors import ( + AmbiguousModelError, + IdCollisionError, + MemoryNotFoundError, +) from slayer.core.models import DatasourceConfig, SlayerModel from slayer.core.query import SlayerQuery from slayer.embeddings.models import Embedding @@ -113,6 +117,30 @@ def _entity_matches_cascade( return entry.startswith(f"{canonical_id}.") +def _fs_equivalence_key(value: str) -> str: + """Identity key under which two ids would address the same file on a + case-insensitive filesystem (macOS / Windows defaults).""" + return value.casefold() + + +def _find_case_colliding_id( + candidate: str, existing: Iterable[str], +) -> str | None: + """Return an existing id that casefold-equals ``candidate`` but is + spelled differently, or ``None``. + + An exact match never counts — same-id saves are upserts. A + raw-different collider is reported even when an exact match also + exists, so a legacy store holding both spellings surfaces the pair on + a save of either one. + """ + key = _fs_equivalence_key(candidate) + for entry in existing: + if entry != candidate and _fs_equivalence_key(entry) == key: + return entry + return None + + def _validate_path_component(value: str, *, kind: str) -> None: """Reject strings that could traverse out of the storage tree or collide with canonical-id namespace boundaries. @@ -174,22 +202,52 @@ async def save_model( ) -> None: """Persist a model. - Runs save-time validation (currently DEV-1410 derived-column cycle - detection) and then delegates to the backend-specific - :meth:`_save_model_impl`. The ``_validate=False`` escape hatch is - for trusted internal callers — currently only the migration - write-back in :meth:`_migrate_and_refine_on_load` — that must - persist legacy data which may not pass current invariants. + Runs save-time validation (case-collision rejection on the model + identity, then derived-column cycle detection) and delegates to + the backend-specific :meth:`_save_model_impl`. The + ``_validate=False`` escape hatch is for trusted internal callers — + currently only the migration write-back in + :meth:`_migrate_and_refine_on_load` — that must persist legacy + data which may not pass current invariants. Validation rules live in this base class so every backend gets them uniformly without duplication; concrete backends must NOT override this method. """ if _validate: + await self._check_model_identity_collision(model) from slayer.engine.column_dependency import validate_no_column_cycles await validate_no_column_cycles(model=model, storage=self) await self._save_model_impl(model) + async def _check_model_identity_collision(self, model: SlayerModel) -> None: + """Reject a model whose identity differs only by case from an + existing one — such ids alias to the same file in the YAML backend + on case-insensitive filesystems, so they are rejected uniformly. + + Checks both path components: ``data_source`` against registered + datasource names and other models' data_sources (they share the + ``models//`` directory namespace), and ``name`` against models + in the same datasource. + """ + identities = await self._list_all_model_identities() + known_ds = {ds for ds, _ in identities} + known_ds.update(await self.list_datasources()) + collide = _find_case_colliding_id(model.data_source, known_ds) + if collide is not None: + raise IdCollisionError( + kind="datasource", new_id=model.data_source, existing_id=collide, + ) + names_in_ds = [n for ds, n in identities if ds == model.data_source] + collide = _find_case_colliding_id(model.name, names_in_ds) + if collide is not None: + raise IdCollisionError( + kind="model", + new_id=model.name, + existing_id=collide, + data_source=model.data_source, + ) + @abstractmethod async def _save_model_impl(self, model: SlayerModel) -> None: """Backend-specific write of ``model`` to durable storage. @@ -392,8 +450,29 @@ async def _migrate_and_refine_on_load( # ---- datasource CRUD --------------------------------------------------- + async def save_datasource(self, datasource: DatasourceConfig) -> None: + """Persist a datasource config. + + Rejects a name that differs only by case from an existing + datasource name — or from a saved model's ``data_source`` (they + share the ``models//`` directory namespace in the YAML + backend) — then delegates to the backend-specific + :meth:`_save_datasource_impl`. Same-name saves are upserts. + Concrete backends must NOT override this method. + """ + existing = set(await self.list_datasources()) + existing.update(ds for ds, _ in await self._list_all_model_identities()) + collide = _find_case_colliding_id(datasource.name, existing) + if collide is not None: + raise IdCollisionError( + kind="datasource", new_id=datasource.name, existing_id=collide, + ) + await self._save_datasource_impl(datasource) + @abstractmethod - async def save_datasource(self, datasource: DatasourceConfig) -> None: ... + async def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: + """Backend-specific write of the datasource config. Concrete + backends implement only this method, not ``save_datasource``.""" @abstractmethod async def get_datasource(self, name: str) -> DatasourceConfig | None: ... @@ -591,6 +670,12 @@ async def _next_memory_seq(self) -> str: no-leading-zero ids count toward the max walk; ``"42abc"`` and ``"001"`` are ignored. Empty corpus → ``"1"``.""" + async def _list_memory_ids(self) -> list[str]: + """Every persisted memory id. The default loads full rows; + backends override with a cheaper id-only listing (directory scan, + ``SELECT id``).""" + return [m.id for m in await self._list_memories_rows(entities=None)] + async def save_memory( self, *, @@ -605,7 +690,8 @@ async def save_memory( * ``id=None`` → allocator picks the next int-shaped id (``str``). * ``id="some-string"`` → user-supplied; rejected on bad charset or empty. Duplicate id → unconditional upsert; ``created_at`` - of the original row is preserved. + of the original row is preserved. An id that differs only by + case from an existing one raises :class:`IdCollisionError`. DEV-1549: ``description`` is an optional compact preview shown by ``search(compact=True)`` and ``inspect_model(compact=True)``. @@ -613,6 +699,11 @@ async def save_memory( """ if id is not None: _validate_memory_id_charset(id) + collide = _find_case_colliding_id(id, await self._list_memory_ids()) + if collide is not None: + raise IdCollisionError( + kind="memory", new_id=id, existing_id=collide, + ) existing = await self._get_memory_row(id) assigned_id = id preserved_created_at = ( diff --git a/slayer/storage/join_sync.py b/slayer/storage/join_sync.py index ddfd76b5..112f65b8 100644 --- a/slayer/storage/join_sync.py +++ b/slayer/storage/join_sync.py @@ -231,8 +231,16 @@ async def update_column_sampled( ) async def save_datasource(self, datasource: DatasourceConfig) -> None: + # Delegate the PUBLIC method so the case-collision check runs once, + # inside the inner backend's template — the wrapper-level template + # would additionally trigger _ensure_reconciled() via + # _list_all_model_identities on a datasource-only flow. return await self._inner.save_datasource(datasource) + async def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: + # Satisfies the ABC; unreachable through the public override above. + return await self._inner._save_datasource_impl(datasource) + async def get_datasource(self, name: str) -> DatasourceConfig | None: return await self._inner.get_datasource(name) @@ -267,6 +275,11 @@ async def _delete_memory_row(self, memory_id: str) -> bool: async def _next_memory_seq(self) -> str: return await self._inner._next_memory_seq() + async def _list_memory_ids(self) -> list[str]: + # The base save_memory template runs at this wrapper level; route + # its id listing to the inner backend's cheap override. + return await self._inner._list_memory_ids() + async def strip_dangling_entities_from_memories( self, *, canonical_id: str, ) -> int: diff --git a/slayer/storage/sqlite_storage.py b/slayer/storage/sqlite_storage.py index 534750df..48cbbde6 100644 --- a/slayer/storage/sqlite_storage.py +++ b/slayer/storage/sqlite_storage.py @@ -13,11 +13,13 @@ import sqlite3 from typing import Any +from slayer.core.errors import IdCollisionError from slayer.core.models import DatasourceConfig, SlayerModel from slayer.core.query import SlayerQuery from slayer.memories.models import Memory, _validate_memory_id_charset from slayer.storage.base import ( StorageBackend, + _find_case_colliding_id, _validate_path_component, _write_sample_fields, ) @@ -347,7 +349,7 @@ async def update_column_sampled( sampled_values=sampled_values, distinct_count=distinct_count, ) - async def save_datasource(self, datasource: DatasourceConfig) -> None: + async def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: await asyncio.to_thread(self._save_datasource_sync, datasource) async def get_datasource(self, name: str) -> DatasourceConfig | None: @@ -434,6 +436,20 @@ def _save_memory_atomic_sync( if memory_id is None: memory_id = self._next_memory_seq_sync_from_conn(conn) else: + # Case-collision check must run inside this transaction: + # this override bypasses the base save_memory template + # where the shared check lives. + ids = [ + row[0] if isinstance(row[0], str) else str(row[0]) + for row in conn.execute("SELECT id FROM memories") + ] + collide = _find_case_colliding_id(memory_id, ids) + if collide is not None: + raise IdCollisionError( + kind="memory", + new_id=memory_id, + existing_id=collide, + ) existing_row = conn.execute( "SELECT data FROM memories WHERE id = ?", (memory_id,), @@ -543,6 +559,14 @@ def _next_memory_seq_sync(self) -> str: async def _next_memory_seq(self) -> str: return await asyncio.to_thread(self._next_memory_seq_sync) + def _list_memory_ids_sync(self) -> list[str]: + with sqlite3.connect(self.db_path) as conn: + rows = conn.execute("SELECT id FROM memories").fetchall() + return [r[0] if isinstance(r[0], str) else str(r[0]) for r in rows] + + async def _list_memory_ids(self) -> list[str]: + return await asyncio.to_thread(self._list_memory_ids_sync) + def _get_memory_sync(self, memory_id: str) -> str | None: with sqlite3.connect(self.db_path) as conn: row = conn.execute( diff --git a/slayer/storage/v4_migration.py b/slayer/storage/v4_migration.py index 7970a38e..52ad1921 100644 --- a/slayer/storage/v4_migration.py +++ b/slayer/storage/v4_migration.py @@ -98,6 +98,47 @@ def _yaml_list_datasource_names(datasources_dir: str) -> list[str]: ] +def _check_layout_case_collisions( + models_dir: str, + planned: list[tuple[str, dict, str, str]], +) -> None: + """Refuse the layout migration when two targets differ only by case: + on a case-insensitive filesystem the second write would clobber the + first. Planned targets are checked against each other and against the + existing v4 subdirectories/files, before anything moves — a failure + leaves every flat file in place. + """ + ds_by_key: dict[str, str] = {} + file_by_key: dict[tuple[str, str], str] = {} + for entry in os.listdir(models_dir): + entry_path = os.path.join(models_dir, entry) + if not os.path.isdir(entry_path): + continue + ds_by_key[entry.casefold()] = entry + for f in os.listdir(entry_path): + if f.endswith((".yaml", ".yml")): + file_by_key[(entry.casefold(), f.casefold())] = os.path.join(entry, f) + for path, _data, ds, filename in planned: + prior_ds = ds_by_key.get(ds.casefold()) + if prior_ds is not None and prior_ds != ds: + raise ValueError( + f"Cannot migrate '{path}' to v4 layout: its datasource " + f"{ds!r} differs only by case from existing {prior_ds!r}. " + f"Rename one, then reopen storage." + ) + ds_by_key[ds.casefold()] = ds + key = (ds.casefold(), filename.casefold()) + target_rel = os.path.join(ds, filename) + prior_file = file_by_key.get(key) + if prior_file is not None and prior_file != target_rel: + raise ValueError( + f"Cannot migrate '{path}' to v4 layout: target " + f"'{target_rel}' differs only by case from existing " + f"'{prior_file}'. Rename one, then reopen storage." + ) + file_by_key[key] = target_rel + + def migrate_yaml_layout(base_dir: str) -> None: """Move flat ``models/.yaml`` files into ``models//``. @@ -121,6 +162,10 @@ def migrate_yaml_layout(base_dir: str) -> None: available = _yaml_list_datasource_names(datasources_dir) + # Pass 1: read every flat file and resolve its datasource without + # moving anything, so the collision check below can veto the whole + # migration while the flat files are still intact. + planned: list[tuple[str, dict, str, str]] = [] for filename in flat_files: path = os.path.join(models_dir, filename) with open(path) as f: @@ -129,6 +174,12 @@ def migrate_yaml_layout(base_dir: str) -> None: if not ds: ds = _resolve_orphan_data_source(name=filename, available_datasources=available) data["data_source"] = ds + planned.append((path, data, ds, filename)) + + _check_layout_case_collisions(models_dir, planned) + + # Pass 2: perform the moves. + for path, data, ds, filename in planned: target_dir = os.path.join(models_dir, ds) os.makedirs(target_dir, exist_ok=True) target_path = os.path.join(target_dir, filename) diff --git a/slayer/storage/yaml_storage.py b/slayer/storage/yaml_storage.py index ef6619ab..33dc952d 100644 --- a/slayer/storage/yaml_storage.py +++ b/slayer/storage/yaml_storage.py @@ -105,6 +105,20 @@ def _atomic_write_text(path: str, text: str) -> None: os.replace(tmp, path) +def _exact_entry_exists(dir_path: str, entry_name: str) -> bool: + """True iff ``dir_path`` contains an entry named exactly ``entry_name``. + + ``os.path.exists`` matches any case variant on a case-insensitive + filesystem, so ``get_model("Orders")`` would silently open + ``orders.yaml``. Comparing against ``os.listdir`` restores exact-id + semantics: a case mismatch reads as "not found". + """ + try: + return entry_name in os.listdir(dir_path) + except (FileNotFoundError, NotADirectoryError): + return False + + def _normalize_legacy_memory_rows( rows: list[dict[str, Any]], ) -> list[dict[str, Any]]: @@ -184,6 +198,26 @@ def migrate_memories_layout(base_dir: str) -> None: ) normalized = _normalize_legacy_memory_rows(rows) mem_dir = os.path.join(base_dir, "memories") + # Reject ids that differ only by case BEFORE writing anything: on a + # case-insensitive filesystem the second ``.md`` write would clobber + # the first and the legacy file would then be deleted — data loss. + # Stems already in memories/ participate too, so a crash-resumed run + # can't clobber a file written by a previous attempt. + id_by_key: dict[str, str] = {} + if os.path.isdir(mem_dir): + for fname in os.listdir(mem_dir): + if fname.endswith(".md"): + stem = fname[: -len(".md")] + id_by_key[stem.casefold()] = stem + for r in normalized: + rid = str(r["id"]) + prior = id_by_key.get(rid.casefold()) + if prior is not None and prior != rid: + raise ValueError( + f"{legacy}: memory ids {prior!r} and {rid!r} differ only by " + f"case and would collide as files. Rename one, then reopen." + ) + id_by_key[rid.casefold()] = rid os.makedirs(mem_dir, exist_ok=True) for r in normalized: mem = Memory.model_validate(r) @@ -267,6 +301,15 @@ async def graph_fingerprint(self) -> str: def _model_path(self, data_source: str, name: str) -> str: return os.path.join(self.models_dir, data_source, f"{name}.yaml") + def _model_entry_exists(self, data_source: str, name: str) -> bool: + """Exact-case existence check for both path components of a model + (the datasource directory and the model file).""" + return _exact_entry_exists( + self.models_dir, data_source, + ) and _exact_entry_exists( + os.path.join(self.models_dir, data_source), f"{name}.yaml", + ) + # ---- model CRUD -------------------------------------------------------- async def _save_model_impl(self, model: SlayerModel) -> None: @@ -299,8 +342,8 @@ async def get_model( if target is None: return None data_source, name = target - path = self._model_path(data_source, name) - if not os.path.exists(path): # NOSONAR(S6549) — name/data_source were sanitized by _resolve_target_or_none above (rejects '..', path separators, NULs); SlayerModel Pydantic validators sanitize the save path + path = self._model_path(data_source, name) # NOSONAR(S6549) — name/data_source were sanitized by _resolve_target_or_none above (rejects '..', path separators, NULs); SlayerModel Pydantic validators sanitize the save path + if not self._model_entry_exists(data_source, name): return None try: with open(path) as f: @@ -319,11 +362,12 @@ async def get_model( async def _delete_model_row( self, *, data_source: str, name: str, ) -> bool: - path = self._model_path(data_source, name) - if os.path.exists(path): - os.remove(path) - return True - return False + # Exact match required: os.remove on a case-insensitive filesystem + # would otherwise delete a case-variant sibling's file. + if not self._model_entry_exists(data_source, name): + return False + os.remove(self._model_path(data_source, name)) + return True async def update_column_sampled( self, @@ -336,7 +380,7 @@ async def update_column_sampled( distinct_count: int | None, ) -> None: path = self._model_path(data_source, model_name) - if not os.path.exists(path): + if not self._model_entry_exists(data_source, model_name): raise ValueError( f"update_column_sampled: model {model_name!r} in datasource " f"{data_source!r} not found." @@ -363,7 +407,7 @@ async def update_column_sampled( # ---- datasource CRUD --------------------------------------------------- - async def save_datasource(self, datasource: DatasourceConfig) -> None: + async def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: path = os.path.join(self.datasources_dir, f"{datasource.name}.yaml") data = datasource.model_dump(mode="json", exclude_none=True) with open(path, "w") as f: @@ -373,7 +417,7 @@ async def get_datasource(self, name: str) -> DatasourceConfig | None: # DEV-1405: sanitize before composing the filesystem path. _validate_path_component(name, kind="datasource name") path = os.path.join(self.datasources_dir, f"{name}.yaml") - if not os.path.exists(path): + if not _exact_entry_exists(self.datasources_dir, f"{name}.yaml"): return None try: with open(path) as f: @@ -397,11 +441,10 @@ async def list_datasources(self) -> list[str]: return result async def _delete_datasource_row(self, name: str) -> bool: - path = os.path.join(self.datasources_dir, f"{name}.yaml") - if os.path.exists(path): - os.remove(path) - return True - return False + if not _exact_entry_exists(self.datasources_dir, f"{name}.yaml"): + return False + os.remove(os.path.join(self.datasources_dir, f"{name}.yaml")) + return True # ---- datasource priority ----------------------------------------------- @@ -465,6 +508,9 @@ def _memory_ids_on_disk(self) -> list[str]: if fname.endswith(".md") ] + async def _list_memory_ids(self) -> list[str]: + return self._memory_ids_on_disk() + async def _next_memory_seq(self) -> str: """DEV-1658: next int-shaped id from the ``memories/`` dir stems. Non-int stems (``help.intro``, ``kb.policy.42``, ``001``) are ignored. @@ -531,6 +577,11 @@ async def _get_memory_row(self, memory_id: str) -> Memory | None: # the check and the open surfaces as FileNotFoundError → treat as # "missing" (return None) rather than crash. path = self._memory_md_path(memory_id) + # The .md content carries no id (the filename is the identity), so + # an exact listdir check is the only way to keep a case-variant + # lookup from opening the wrong file on a case-insensitive FS. + if not _exact_entry_exists(self._memories_dir, f"{memory_id}.md"): + return None try: with open(path, encoding="utf-8") as f: # NOSONAR(S7493) — sync I/O in async by design return _md_to_memory(memory_id, f.read()) @@ -560,7 +611,7 @@ async def _list_memories_rows( async def _delete_memory_row(self, memory_id: str) -> bool: with self._memories_file_lock(): path = self._memory_md_path(memory_id) - if not os.path.exists(path): + if not _exact_entry_exists(self._memories_dir, f"{memory_id}.md"): return False os.remove(path) return True diff --git a/tests/test_api_server.py b/tests/test_api_server.py index bebd0efc..5656cf60 100644 --- a/tests/test_api_server.py +++ b/tests/test_api_server.py @@ -224,6 +224,17 @@ def test_delete_nonexistent(self, client: TestClient) -> None: resp = client.delete("/datasources/nope") assert resp.status_code == 404 + def test_case_variant_name_returns_400(self, client: TestClient) -> None: + resp = client.post( + "/datasources", json={"name": "mydb", "type": "postgres"}, + ) + assert resp.status_code == 200 + resp = client.post( + "/datasources", json={"name": "MyDB", "type": "postgres"}, + ) + assert resp.status_code == 400 + assert "differs only by case" in resp.json()["detail"] + class TestIngestEndpoint: """``POST /ingest`` translates user-correctable config errors into 422s. diff --git a/tests/test_id_collision.py b/tests/test_id_collision.py new file mode 100644 index 00000000..52ec0e11 --- /dev/null +++ b/tests/test_id_collision.py @@ -0,0 +1,326 @@ +"""Case-collision rejection for datasource / model / memory ids. + +Ids become filenames in the YAML backend, where ids differing only by +case address the same file on case-insensitive filesystems (macOS / +Windows defaults). Saves of a case-variant id are rejected uniformly on +every backend so stores stay portable across backends and platforms. +Exact-id re-saves remain upserts. +""" + +from __future__ import annotations + +import json +import os +import sqlite3 +import tempfile +from collections.abc import Iterator + +import pytest +import yaml + +from slayer.core.errors import IdCollisionError +from slayer.core.models import DatasourceConfig, SlayerModel +from slayer.memories.models import Memory +from slayer.storage.base import ( + StorageBackend, + _find_case_colliding_id, + _fs_equivalence_key, + resolve_storage, +) +from slayer.storage.sqlite_storage import SQLiteStorage +from slayer.storage.yaml_storage import YAMLStorage + + +@pytest.fixture(params=["yaml", "sqlite"]) +def storage(request: pytest.FixtureRequest) -> Iterator[StorageBackend]: + with tempfile.TemporaryDirectory() as tmpdir: + if request.param == "yaml": + yield YAMLStorage(base_dir=tmpdir) + else: + yield SQLiteStorage(db_path=os.path.join(tmpdir, "test.db")) + + +def _ds(name: str, host: str = "h") -> DatasourceConfig: + return DatasourceConfig(name=name, type="postgres", host=host) + + +def _model(name: str, data_source: str = "db", table: str = "t") -> SlayerModel: + return SlayerModel(name=name, data_source=data_source, sql_table=table) + + +# --------------------------------------------------------------------------- +# Pure helpers +# --------------------------------------------------------------------------- + + +class TestHelpers: + def test_key_casefolds(self) -> None: + assert _fs_equivalence_key("Orders") == _fs_equivalence_key("orders") + assert _fs_equivalence_key("a") != _fs_equivalence_key("b") + + def test_exact_match_is_not_a_collision(self) -> None: + assert _find_case_colliding_id("orders", ["orders"]) is None + + def test_case_variant_found(self) -> None: + assert _find_case_colliding_id("Orders", ["orders"]) == "orders" + + def test_variant_reported_even_with_exact_match(self) -> None: + # A legacy store holding both spellings must surface the pair on + # a save of either one. + assert _find_case_colliding_id("orders", ["orders", "Orders"]) == "Orders" + + def test_unrelated_ids_do_not_collide(self) -> None: + assert _find_case_colliding_id("orders", ["customers"]) is None + + +# --------------------------------------------------------------------------- +# Datasources +# --------------------------------------------------------------------------- + + +class TestDatasourceCollision: + async def test_case_variant_rejected(self, storage: StorageBackend) -> None: + await storage.save_datasource(_ds("db", host="first")) + with pytest.raises(IdCollisionError) as exc_info: + await storage.save_datasource(_ds("DB", host="second")) + assert exc_info.value.kind == "datasource" + assert exc_info.value.new_id == "DB" + assert exc_info.value.existing_id == "db" + loaded = await storage.get_datasource("db") + assert loaded.host == "first" + + async def test_exact_resave_upserts(self, storage: StorageBackend) -> None: + await storage.save_datasource(_ds("db", host="first")) + await storage.save_datasource(_ds("db", host="second")) + loaded = await storage.get_datasource("db") + assert loaded.host == "second" + + async def test_collides_with_model_data_source( + self, storage: StorageBackend, + ) -> None: + # Models saved under an orphan datasource reserve its name: on + # YAML they share the models// directory namespace. + await storage.save_model(_model("orders", data_source="db")) + with pytest.raises(IdCollisionError): + await storage.save_datasource(_ds("DB")) + # An exactly-named config for the orphan datasource is fine. + await storage.save_datasource(_ds("db")) + + +# --------------------------------------------------------------------------- +# Models +# --------------------------------------------------------------------------- + + +class TestModelCollision: + async def test_case_variant_name_rejected( + self, storage: StorageBackend, + ) -> None: + await storage.save_model(_model("orders", table="first")) + with pytest.raises(IdCollisionError) as exc_info: + await storage.save_model(_model("Orders", table="second")) + assert exc_info.value.kind == "model" + assert exc_info.value.new_id == "Orders" + assert exc_info.value.existing_id == "orders" + assert exc_info.value.data_source == "db" + loaded = await storage.get_model("orders", data_source="db") + assert loaded.sql_table == "first" + + async def test_exact_resave_upserts(self, storage: StorageBackend) -> None: + await storage.save_model(_model("orders", table="first")) + await storage.save_model(_model("orders", table="second")) + loaded = await storage.get_model("orders", data_source="db") + assert loaded.sql_table == "second" + + async def test_same_name_in_other_datasource_ok( + self, storage: StorageBackend, + ) -> None: + # The name check is scoped to one datasource; case variants across + # datasources live in different directories. + await storage.save_model(_model("orders", data_source="db_a")) + await storage.save_model(_model("Orders", data_source="db_b")) + assert await storage.get_model("orders", data_source="db_a") is not None + assert await storage.get_model("Orders", data_source="db_b") is not None + + async def test_case_variant_data_source_rejected( + self, storage: StorageBackend, + ) -> None: + await storage.save_datasource(_ds("db")) + with pytest.raises(IdCollisionError) as exc_info: + await storage.save_model(_model("orders", data_source="DB")) + assert exc_info.value.kind == "datasource" + + async def test_data_source_vs_other_models_rejected( + self, storage: StorageBackend, + ) -> None: + await storage.save_model(_model("orders", data_source="db")) + with pytest.raises(IdCollisionError): + await storage.save_model(_model("customers", data_source="DB")) + + async def test_validate_false_bypasses( + self, storage: StorageBackend, + ) -> None: + # Trusted internal callers (the migration write-back) must stay + # able to re-persist legacy data that predates the check. + await storage.save_model(_model("orders")) + await storage.save_model(_model("Orders"), _validate=False) + + +# --------------------------------------------------------------------------- +# Memories (the save-time rejection itself is covered per-backend in +# test_memory_string_ids.py::test_case_colliding_ids_rejected) +# --------------------------------------------------------------------------- + + +class TestMemoryCollision: + async def test_exact_upsert_allowed(self, storage: StorageBackend) -> None: + await storage.save_memory( + id="kb.x", learning="one", entities=["mydb.orders"], + ) + await storage.save_memory( + id="kb.x", learning="two", entities=["mydb.orders"], + ) + assert (await storage.get_memory("kb.x")).learning == "two" + + async def test_error_attrs(self, storage: StorageBackend) -> None: + await storage.save_memory( + id="Kb.X", learning="one", entities=["mydb.orders"], + ) + with pytest.raises(IdCollisionError) as exc_info: + await storage.save_memory( + id="kb.x", learning="two", entities=["mydb.orders"], + ) + assert exc_info.value.kind == "memory" + assert exc_info.value.new_id == "kb.x" + assert exc_info.value.existing_id == "Kb.X" + + +# --------------------------------------------------------------------------- +# Legacy stores that already hold a colliding pair (created pre-check, on +# a case-sensitive filesystem): a save of EITHER spelling must raise. +# --------------------------------------------------------------------------- + + +class TestLegacyCollidingPair: + async def test_sqlite_memory_pair_blocks_both(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + db_path = os.path.join(tmpdir, "test.db") + storage = SQLiteStorage(db_path=db_path) + with sqlite3.connect(db_path) as conn: + for mid, learning in (("X", "upper"), ("x", "lower")): + m = Memory(id=mid, learning=learning, entities=[]) + conn.execute( + "INSERT INTO memories (id, data) VALUES (?, ?)", + (mid, json.dumps(m.model_dump(mode="json"))), + ) + for mid in ("X", "x"): + with pytest.raises(IdCollisionError): + await storage.save_memory( + id=mid, learning="update", entities=[], + ) + # Both rows are still intact. + assert (await storage.get_memory("X")).learning == "upper" + assert (await storage.get_memory("x")).learning == "lower" + + async def test_sqlite_model_pair_blocks_both(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + db_path = os.path.join(tmpdir, "test.db") + storage = SQLiteStorage(db_path=db_path) + with sqlite3.connect(db_path) as conn: + for name in ("orders", "Orders"): + data = _model(name).model_dump(mode="json", exclude_none=True) + conn.execute( + "INSERT INTO models (data_source, name, data) " + "VALUES (?, ?, ?)", + ("db", name, json.dumps(data)), + ) + for name in ("orders", "Orders"): + with pytest.raises(IdCollisionError): + await storage.save_model(_model(name)) + + +# --------------------------------------------------------------------------- +# The production wrapper (resolve_storage → JoinSyncStorage) propagates +# the rejection through every save surface. +# --------------------------------------------------------------------------- + + +class TestJoinSyncWrapped: + async def test_collisions_raise_through_wrapper(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + storage = resolve_storage(tmpdir) + await storage.save_datasource(_ds("db")) + with pytest.raises(IdCollisionError): + await storage.save_datasource(_ds("DB")) + await storage.save_model(_model("orders")) + with pytest.raises(IdCollisionError): + await storage.save_model(_model("Orders")) + await storage.save_memory( + id="X", learning="a", entities=["mydb.orders"], + ) + with pytest.raises(IdCollisionError): + await storage.save_memory( + id="x", learning="b", entities=["mydb.orders"], + ) + + +# --------------------------------------------------------------------------- +# Layout migrations refuse case-colliding targets before writing anything +# --------------------------------------------------------------------------- + + +class TestMigrationPreChecks: + def test_legacy_memories_yaml_case_pair_refused(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + legacy = os.path.join(tmpdir, "memories.yaml") + with open(legacy, "w") as f: + yaml.safe_dump( + [ + {"id": "X", "learning": "upper", "entities": []}, + {"id": "x", "learning": "lower", "entities": []}, + ], + f, + ) + with pytest.raises(ValueError, match="differ only by"): + YAMLStorage(base_dir=tmpdir) + # Legacy file preserved; no .md written. + assert os.path.exists(legacy) + mem_dir = os.path.join(tmpdir, "memories") + md_files = ( + [f for f in os.listdir(mem_dir) if f.endswith(".md")] + if os.path.isdir(mem_dir) + else [] + ) + assert md_files == [] + + def test_flat_models_case_variant_datasources_refused(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + models_dir = os.path.join(tmpdir, "models") + os.makedirs(models_dir) + for fname, ds in (("a.yaml", "DB"), ("b.yaml", "db")): + with open(os.path.join(models_dir, fname), "w") as f: + yaml.safe_dump( + {"name": fname[:-5], "data_source": ds, "sql_table": "t"}, + f, + ) + with pytest.raises(ValueError, match="differs only by case"): + YAMLStorage(base_dir=tmpdir) + # Flat files untouched; no subdirectory created. + assert sorted(os.listdir(models_dir)) == ["a.yaml", "b.yaml"] + + def test_flat_model_vs_existing_v4_case_variant_refused(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + models_dir = os.path.join(tmpdir, "models") + os.makedirs(os.path.join(models_dir, "db")) + with open(os.path.join(models_dir, "db", "Orders.yaml"), "w") as f: + yaml.safe_dump( + {"name": "Orders", "data_source": "db", "sql_table": "t"}, f, + ) + with open(os.path.join(models_dir, "orders.yaml"), "w") as f: + yaml.safe_dump( + {"name": "orders", "data_source": "db", "sql_table": "t"}, f, + ) + with pytest.raises(ValueError, match="differs only by case"): + YAMLStorage(base_dir=tmpdir) + assert os.path.exists(os.path.join(models_dir, "orders.yaml")) + assert os.path.exists(os.path.join(models_dir, "db", "Orders.yaml")) diff --git a/tests/test_memory_string_ids.py b/tests/test_memory_string_ids.py index d5d6538a..1e6d3fd9 100644 --- a/tests/test_memory_string_ids.py +++ b/tests/test_memory_string_ids.py @@ -14,6 +14,7 @@ import pytest +from slayer.core.errors import IdCollisionError from slayer.memories.models import Memory from slayer.storage.base import StorageBackend from slayer.storage.sqlite_storage import SQLiteStorage @@ -179,19 +180,24 @@ async def test_save_with_user_id(self, storage: StorageBackend) -> None: loaded = await storage.get_memory("kb.policy.42") assert loaded.learning == "x" - async def test_case_sensitive_ids( + async def test_case_colliding_ids_rejected( self, storage: StorageBackend, ) -> None: + # Ids differing only by case would alias to the same file in the + # YAML backend on case-insensitive filesystems; rejected uniformly + # on every backend so stores stay portable. await storage.save_memory( id="X", learning="upper", entities=["mydb.orders"], ) - await storage.save_memory( - id="x", learning="lower", entities=["mydb.orders"], - ) + with pytest.raises(IdCollisionError) as exc_info: + await storage.save_memory( + id="x", learning="lower", entities=["mydb.orders"], + ) + assert exc_info.value.new_id == "x" + assert exc_info.value.existing_id == "X" + # The original memory is untouched. upper = await storage.get_memory("X") - lower = await storage.get_memory("x") assert upper.learning == "upper" - assert lower.learning == "lower" async def test_zero_user_id_distinct_from_auto( self, storage: StorageBackend, diff --git a/tests/test_yaml_exact_entry.py b/tests/test_yaml_exact_entry.py new file mode 100644 index 00000000..7861d28a --- /dev/null +++ b/tests/test_yaml_exact_entry.py @@ -0,0 +1,127 @@ +"""YAML read/delete exact-name verification. + +On a case-insensitive filesystem ``open`` / ``os.path.exists`` match any +case variant of a filename, so a lookup for ``Orders`` would silently hit +``orders.yaml`` — and a delete would remove it. The YAML backend compares +against ``os.listdir`` instead, so a case mismatch behaves as "not +found". These assertions hold on both case-sensitive and +case-insensitive dev machines. +""" + +from __future__ import annotations + +import os +import tempfile +from collections.abc import Iterator + +import pytest + +from slayer.core.errors import MemoryNotFoundError +from slayer.core.models import DatasourceConfig, SlayerModel +from slayer.storage.yaml_storage import YAMLStorage, _exact_entry_exists + + +@pytest.fixture +def base_dir() -> Iterator[str]: + with tempfile.TemporaryDirectory() as tmpdir: + yield tmpdir + + +@pytest.fixture +def storage(base_dir: str) -> YAMLStorage: + return YAMLStorage(base_dir=base_dir) + + +class TestExactEntryExists: + def test_exact_name(self, base_dir: str) -> None: + open(os.path.join(base_dir, "orders.yaml"), "w").close() + assert _exact_entry_exists(base_dir, "orders.yaml") is True + + def test_case_variant_not_found(self, base_dir: str) -> None: + open(os.path.join(base_dir, "orders.yaml"), "w").close() + assert _exact_entry_exists(base_dir, "Orders.yaml") is False + + def test_missing_dir(self, base_dir: str) -> None: + assert _exact_entry_exists(os.path.join(base_dir, "nope"), "x") is False + + +class TestModelReads: + @pytest.fixture(autouse=True) + async def _seed(self, storage: YAMLStorage) -> None: + await storage.save_model( + SlayerModel(name="orders", data_source="db", sql_table="t"), + ) + + async def test_get_case_variant_name_returns_none( + self, storage: YAMLStorage, + ) -> None: + assert await storage.get_model("Orders", data_source="db") is None + assert await storage.get_model("orders", data_source="db") is not None + + async def test_get_case_variant_data_source_returns_none( + self, storage: YAMLStorage, + ) -> None: + assert await storage.get_model("orders", data_source="DB") is None + + async def test_delete_case_variant_is_noop( + self, storage: YAMLStorage, + ) -> None: + assert await storage.delete_model("Orders", data_source="db") is False + # The real file is still there. + assert await storage.get_model("orders", data_source="db") is not None + + async def test_update_column_sampled_case_variant_raises( + self, storage: YAMLStorage, + ) -> None: + with pytest.raises(ValueError, match="not found"): + await storage.update_column_sampled( + data_source="db", + model_name="Orders", + column_name="c", + sampled=None, + sampled_values=None, + distinct_count=None, + ) + + +class TestDatasourceReads: + @pytest.fixture(autouse=True) + async def _seed(self, storage: YAMLStorage) -> None: + await storage.save_datasource( + DatasourceConfig(name="db", type="postgres", host="h"), + ) + + async def test_get_case_variant_returns_none( + self, storage: YAMLStorage, + ) -> None: + assert await storage.get_datasource("DB") is None + assert await storage.get_datasource("db") is not None + + async def test_delete_case_variant_is_noop( + self, storage: YAMLStorage, + ) -> None: + assert await storage.delete_datasource("DB") is False + assert await storage.get_datasource("db") is not None + + +class TestMemoryReads: + @pytest.fixture(autouse=True) + async def _seed(self, storage: YAMLStorage) -> None: + await storage.save_memory( + id="x", learning="lower", entities=["mydb.orders"], + ) + + async def test_get_case_variant_not_found( + self, storage: YAMLStorage, + ) -> None: + with pytest.raises(MemoryNotFoundError): + await storage.get_memory("X") + assert (await storage.get_memory("x")).learning == "lower" + + async def test_delete_case_variant_not_found( + self, storage: YAMLStorage, + ) -> None: + with pytest.raises(MemoryNotFoundError): + await storage.delete_memory("X") + # The real file is still there. + assert (await storage.get_memory("x")).learning == "lower" From e17e7db2085fae1fe38017e60b7c0ee615381a65 Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 22 Jul 2026 18:07:06 +0400 Subject: [PATCH 2/5] Keep save_datasource backend-implemented; expose the check as a public helper The template-method split (save_datasource delegating to a new abstract _save_datasource_impl) forced third-party backends to rename their overrides on upgrade. Revert it: save_datasource stays the abstract method backends implement directly, and the case-collision check moves into a public base helper, check_datasource_id_collision(name), which the built-in YAML/SQLite implementations call before writing. Custom backends opt into the same protection by calling it from their own save_datasource. Claude-Session: https://claude.ai/code/session_011sbQKNXcUtGAsCXtmJXHvR --- CLAUDE.md | 2 +- docs/configuration/storage.md | 12 ++++++----- slayer/storage/base.py | 35 +++++++++++++++++--------------- slayer/storage/join_sync.py | 10 ++------- slayer/storage/sqlite_storage.py | 3 ++- slayer/storage/yaml_storage.py | 3 ++- 6 files changed, 33 insertions(+), 32 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 13d93d18..1021b5dc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,7 +53,7 @@ poetry run ruff check slayer/ tests/ - **Schema versioning**: `SlayerModel`, `SlayerQuery`, and `DatasourceConfig` carry a `version: int` (currently `7` for `SlayerModel`, `3` for `SlayerQuery`, `1` for `DatasourceConfig`). On load, older versions are upgraded via the converter chain in `slayer/storage/migrations.py` before Pydantic validates the dict. Saves always emit the current version. The hook is on the Pydantic class itself (`@model_validator(mode="before")`), so every storage backend — YAML, SQLite, third-party backends, plus MCP/API/dbt entry points — gets migrations automatically without backend changes. The v1→v2 converter (in `slayer/storage/v2_migration.py`) merges v1 `dimensions`+`measures` into v2 `columns`, repurposes `measures` to hold `ModelMeasure` formulas, and renames `SlayerQuery.fields`→`measures`. The v2→v3 converter (in `slayer/storage/v3_migration.py`) drops the legacy `dry_run`/`explain` fields from `SlayerQuery` (they are now engine kwargs only — `engine.execute(query, dry_run=..., explain=...)`) and walks `SlayerModel.source_queries` entries through the SlayerQuery chain. The v3→v4 converter (in `slayer/storage/v4_migration.py`) requires non-empty `data_source` on table-backed SlayerModel dicts (`sql_table` or `sql` mode); query-backed models (`source_queries` set) are exempt because their `data_source` is filled by `engine._validate_and_populate_cache` from the resolved virtual model before save. The v4 converter also ships layout migrators that move legacy `models/.yaml` flat files into `models//.yaml` and rebuild the SQLite `models` table with a composite `(data_source, name)` primary key. The v4→v5 converter (in `slayer/storage/v5_migration.py`, DEV-1361) coarse-renames `Column.type` legacy values to the new sqlglot-aligned vocabulary (`string→TEXT`, `number→DOUBLE`, `time→TIMESTAMP`, etc.) and strips the dead aggregation pseudo-types (`count`/`sum`/...). The v5→v6 converter (in `slayer/storage/v6_migration.py`, DEV-1375) is a no-op forward — v6 introduces a single new optional field, `Column.sampled: Optional[str]`, that caches the per-column sample-value snapshot consumed by [`search`](docs/concepts/search.md) and `inspect_model`. The v6→v7 converter (in `slayer/storage/v7_migration.py`, DEV-1480) is also a no-op forward — v7 adds two more `Column` fields: `sampled_values: Optional[List[str]]` (the structured top-50-by-frequency list, paired with the existing text `sampled`) and `distinct_count: Optional[int]` (true cardinality at profile time, surfaced via a secondary `count_distinct` query on overflow). Storage backends additionally introspect each model's datasource on first load and refine `DOUBLE → INT` for base columns whose live SQL type is integer (`slayer/storage/type_refinement.py`); the refined model is written back so subsequent loads skip both steps. `SlayerQuery` v3 sets `extra="forbid"` so unknown fields raise. See [docs/concepts/models.md](docs/concepts/models.md#schema-versioning). - **DataType / CAST emission** (v5, DEV-1361): `DataType` values match sqlglot's `exp.DataType.Type` byte-for-byte: `TEXT`, `INT`, `DOUBLE`, `BOOLEAN`, `DATE`, `TIMESTAMP`. Auto-ingestion narrows integer DB types (INTEGER/BIGINT/SERIAL/INT8…/UINT8…) to `INT`, and floats/numerics (FLOAT/DOUBLE/DECIMAL with scale>0) to `DOUBLE`; NUMERIC(p,0) and DECIMAL(p,0) are integer-shaped and narrow to `INT`. The SQL generator wraps **non-bare** `Column.sql` (function calls, arithmetic, CASE WHEN) in `CAST(... AS )` driven by `Column.type`; bare identifiers and `sql=None` are emitted unchanged. `TEXT` is a no-op (skipped, since `CAST AS TEXT` is cosmetic and doesn't unwrap SQLite's JSON-quoted strings anyway). `ModelMeasure.type` (also `Optional[DataType]`) declares the formula's result type; when set, the aggregation expression is wrapped in an outer CAST. Lenient `before`-validators on `Column.type` and `ModelMeasure.type` absorb legacy lowercase agent input (`"string"` → `TEXT`, `"number"` → `DOUBLE`, etc.) and silently drop dropped pseudo-types. `slayer storage migrate-types --dry-run [--data-source X]` runs the storage refinement step explicitly for batch / inspectable usage. Schema-drift detection (`data_type_bucket`) keeps `INT` and `DOUBLE` in the same `"number"` bucket so a `DOUBLE`-typed persisted column does not flag as drift against an `INT` live column. - **Datasource-scoped storage** (v4, DEV-1330): Models are keyed by `(data_source, name)`, not bare `name`. Two datasources can share a table name without collision. `storage.get_model(name, data_source=None)` and `storage.delete_model(name, data_source=None)` resolve bare names by: (1) returning the unique match if exactly one model has that name, (2) walking `storage.get_datasource_priority()` to pick the first datasource in the priority list that has the name, (3) raising `slayer.core.errors.AmbiguousModelError` otherwise. The priority list is configured via `storage.set_datasource_priority(["db_a", ...])`, the MCP `set_datasource_priority` tool, the REST `PUT /datasources/priority`, or the `slayer datasources priority` CLI subcommand. `engine.execute(query, data_source=...)` passes a hint that wins over the priority list. Joins resolve targets within the parent model's `data_source` only — cross-datasource joins are not auto-mirrored. A sibling Linear issue ([DEV-1342](https://linear.app/motley-ai/issue/DEV-1342)) tracks whether to add `datasource.model_name` dot syntax inside query strings. -- **Case-collision rejection** (issue #249): saving a datasource, model, or memory whose id differs only by letter case (`casefold()`-equal, raw-different) from an existing one raises `slayer.core.errors.IdCollisionError(SlayerError, ValueError)` — attrs `kind`/`new_id`/`existing_id`/`data_source`. Ids become filenames in the YAML backend (case variants alias on macOS/Windows); the check is enforced in the `StorageBackend` base templates so EVERY backend rejects on EVERY platform and stores stay portable. Exact-id re-saves remain upserts; a legacy store already holding a colliding pair raises on a save of either spelling. Model saves check both components: `data_source` against registered datasource names ∪ other models' data_sources (shared `models//` dir namespace), and `name` within the same datasource. `save_datasource` is now a base template method delegating to abstract `_save_datasource_impl` (backends implement only the impl — same pattern as `save_model`/`_save_model_impl`); `save_memory` checks via the cheap `_list_memory_ids()` primitive (YAML: listdir; SQLite: `SELECT id`; JoinSync: delegation). `SQLiteStorage.save_memory` bypasses the base template, so the same check is duplicated inside `_save_memory_atomic_sync`'s transaction. `save_model(_validate=False)` (migration write-back) bypasses the check. YAML reads/deletes verify the EXACT directory entry via `os.listdir` comparison (`_exact_entry_exists`) — a case-variant `get_model`/`get_datasource`/`get_memory` returns not-found and a delete is a no-op instead of silently hitting the wrong file. `migrate_memories_layout` and `migrate_yaml_layout` pre-check collisions before writing anything (legacy files preserved on failure). Surfacing: REST `POST /datasources` → 400 (model/memory endpoints already mapped ValueError→400); MCP `create_datasource` returns a friendly message and its auto-ingest loop reports per-model save failures without aborting; CLI prints and exits (or skips-and-continues in ingest loops). +- **Case-collision rejection** (issue #249): saving a datasource, model, or memory whose id differs only by letter case (`casefold()`-equal, raw-different) from an existing one raises `slayer.core.errors.IdCollisionError(SlayerError, ValueError)` — attrs `kind`/`new_id`/`existing_id`/`data_source`. Ids become filenames in the YAML backend (case variants alias on macOS/Windows); the check is enforced in the `StorageBackend` base templates so EVERY backend rejects on EVERY platform and stores stay portable. Exact-id re-saves remain upserts; a legacy store already holding a colliding pair raises on a save of either spelling. Model saves check both components: `data_source` against registered datasource names ∪ other models' data_sources (shared `models//` dir namespace), and `name` within the same datasource. `save_datasource` stays a backend-implemented abstract method (no rename — third-party overrides keep working); the datasource check lives in the public base helper `check_datasource_id_collision(name)`, which the built-in YAML/SQLite implementations call before writing (custom backends opt in the same way). `save_memory` checks via the cheap `_list_memory_ids()` primitive (YAML: listdir; SQLite: `SELECT id`; JoinSync: delegation). `SQLiteStorage.save_memory` bypasses the base template, so the same check is duplicated inside `_save_memory_atomic_sync`'s transaction. `save_model(_validate=False)` (migration write-back) bypasses the check. YAML reads/deletes verify the EXACT directory entry via `os.listdir` comparison (`_exact_entry_exists`) — a case-variant `get_model`/`get_datasource`/`get_memory` returns not-found and a delete is a no-op instead of silently hitting the wrong file. `migrate_memories_layout` and `migrate_yaml_layout` pre-check collisions before writing anything (legacy files preserved on failure). Surfacing: REST `POST /datasources` → 400 (model/memory endpoints already mapped ValueError→400); MCP `create_datasource` returns a friendly message and its auto-ingest loop reports per-model save failures without aborting; CLI prints and exits (or skips-and-continues in ingest loops). - **Source modes**: a `SlayerModel` has exactly one source: `sql_table` (physical table), `sql` (explicit SQL subquery), or `source_queries` (query-backed: `List[SlayerQuery]`). Validators enforce mutual exclusivity, reject empty `source_queries=[]`, require `name` on every non-final stage, and reject duplicate stage names. `SlayerModel.source_queries` entries given as dicts are auto-parsed into `SlayerQuery` instances by a Pydantic before-validator. - **Stages form a DAG, not just a chain**: any stage in `source_queries` or a runtime query list may use a named sibling as `source_model` or as `joins.target_model`. The **runtime list path** (`engine.execute(query=[...])`, CLI `slayer query @file.json`, MCP `query_nested`) **auto-sorts** the input via `SlayerQueryEngine._topologically_order_queries` — a hand-rolled Kahn's algorithm (no `graphlib` dep) that reorders so every stage appears after the siblings it references. The last entry of the input stays last as the entry point / DAG root, so the result returned doesn't depend on sort order. Validations performed up front: missing `name` on a non-final entry; duplicate names; self-references; the root being referenced by any other stage (root must be the dependency sink); cycles. Unreachable utility sub-queries are accepted — they flow through the sort but are silently dropped from the emitted SQL since nothing resolves them. Stored `SlayerModel.source_queries` retains strict-order semantics (relies on `_scope_named_queries_to_prior` plus the `_forbidden_sibling_refs_var` ContextVar in `_resolve_model_inner` / `_resolve_join_target`) — YAML-defined source_queries are read top-to-bottom and the strict-order error pins typos at resolve time. The runtime list's reference extractor walks `source_model` (string sibling refs), `ModelExtension.source_name`, and `joins[].target_model` from both dict and typed shapes; formula-only references resolve correctly because the explicit join they require is what the extractor picks up. - **Query-backed models**: `SlayerModel.query_variables: Dict[str, Any]` provides defaults for `{var}` placeholders in `source_queries`. `engine.create_model_from_query(query, name, variables=..., save=True)` saves a query as a query-backed model and populates the `columns` + `backing_query_sql` cache. `engine.save_model(model)` is the engine-side save helper that runs source-mode validation + cache refresh + persistence; user-supplied `columns` and `backing_query_sql` on a query-backed model are rejected at save with a clear error. The cache is refreshed only on save paths (`engine.save_model` / `create_model_from_query(save=True)`); `engine.execute` never writes to storage, even on stale or empty caches (closes #74 — `tests/test_query_backed_models.py::test_execute_never_writes_to_storage` pins this). diff --git a/docs/configuration/storage.md b/docs/configuration/storage.md index 391ff253..1aad00cc 100644 --- a/docs/configuration/storage.md +++ b/docs/configuration/storage.md @@ -113,10 +113,9 @@ from slayer.storage.base import StorageBackend from slayer.core.models import SlayerModel, DatasourceConfig class MyCustomStorage(StorageBackend): - # Models are keyed by (data_source, name). ``save_model`` and - # ``save_datasource`` are template methods on the base class (they run - # shared validation, e.g. case-collision rejection) — backends implement - # only the ``_impl`` writes. + # Models are keyed by (data_source, name). ``save_model`` is a template + # method on the base class (it runs shared validation — cycle detection, + # case-collision rejection); backends implement only ``_save_model_impl``. def _save_model_impl(self, model: SlayerModel) -> None: ... def _list_all_model_identities(self) -> list[tuple[str, str]]: ... def get_model(self, name: str, data_source: str | None = None) -> SlayerModel | None: ... @@ -125,7 +124,10 @@ class MyCustomStorage(StorageBackend): # ``StorageBackend`` provides default implementations of ``list_models`` # and ``resolve_model_identity`` on top of ``_list_all_model_identities``. - def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: ... + # Call ``await self.check_datasource_id_collision(datasource.name)`` + # first to get the same case-collision protection as the built-in + # backends (optional but recommended). + def save_datasource(self, datasource: DatasourceConfig) -> None: ... def get_datasource(self, name: str) -> DatasourceConfig | None: ... def list_datasources(self) -> list[str]: ... def _delete_datasource_row(self, name: str) -> bool: ... diff --git a/slayer/storage/base.py b/slayer/storage/base.py index bc244069..635e4a04 100644 --- a/slayer/storage/base.py +++ b/slayer/storage/base.py @@ -450,29 +450,32 @@ async def _migrate_and_refine_on_load( # ---- datasource CRUD --------------------------------------------------- + @abstractmethod async def save_datasource(self, datasource: DatasourceConfig) -> None: - """Persist a datasource config. - - Rejects a name that differs only by case from an existing - datasource name — or from a saved model's ``data_source`` (they - share the ``models//`` directory namespace in the YAML - backend) — then delegates to the backend-specific - :meth:`_save_datasource_impl`. Same-name saves are upserts. - Concrete backends must NOT override this method. + """Persist a datasource config (upsert by exact name). + + Implementations should call :meth:`check_datasource_id_collision` + with ``datasource.name`` before writing so case-variant names are + rejected — the built-in YAML and SQLite backends do. + """ + + async def check_datasource_id_collision(self, name: str) -> None: + """Raise :class:`IdCollisionError` when ``name`` differs only by + case from an existing datasource name or from a saved model's + ``data_source`` (they share the ``models//`` directory + namespace in the YAML backend). An exact match is fine — saves + are upserts. + + Public so custom backends can opt into the same protection from + their own ``save_datasource`` implementations. """ existing = set(await self.list_datasources()) existing.update(ds for ds, _ in await self._list_all_model_identities()) - collide = _find_case_colliding_id(datasource.name, existing) + collide = _find_case_colliding_id(name, existing) if collide is not None: raise IdCollisionError( - kind="datasource", new_id=datasource.name, existing_id=collide, + kind="datasource", new_id=name, existing_id=collide, ) - await self._save_datasource_impl(datasource) - - @abstractmethod - async def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: - """Backend-specific write of the datasource config. Concrete - backends implement only this method, not ``save_datasource``.""" @abstractmethod async def get_datasource(self, name: str) -> DatasourceConfig | None: ... diff --git a/slayer/storage/join_sync.py b/slayer/storage/join_sync.py index 112f65b8..24c6c2f2 100644 --- a/slayer/storage/join_sync.py +++ b/slayer/storage/join_sync.py @@ -231,16 +231,10 @@ async def update_column_sampled( ) async def save_datasource(self, datasource: DatasourceConfig) -> None: - # Delegate the PUBLIC method so the case-collision check runs once, - # inside the inner backend's template — the wrapper-level template - # would additionally trigger _ensure_reconciled() via - # _list_all_model_identities on a datasource-only flow. + # Pure delegation — the case-collision check runs once, inside the + # inner backend's implementation. return await self._inner.save_datasource(datasource) - async def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: - # Satisfies the ABC; unreachable through the public override above. - return await self._inner._save_datasource_impl(datasource) - async def get_datasource(self, name: str) -> DatasourceConfig | None: return await self._inner.get_datasource(name) diff --git a/slayer/storage/sqlite_storage.py b/slayer/storage/sqlite_storage.py index 48cbbde6..f7f530f7 100644 --- a/slayer/storage/sqlite_storage.py +++ b/slayer/storage/sqlite_storage.py @@ -349,7 +349,8 @@ async def update_column_sampled( sampled_values=sampled_values, distinct_count=distinct_count, ) - async def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: + async def save_datasource(self, datasource: DatasourceConfig) -> None: + await self.check_datasource_id_collision(datasource.name) await asyncio.to_thread(self._save_datasource_sync, datasource) async def get_datasource(self, name: str) -> DatasourceConfig | None: diff --git a/slayer/storage/yaml_storage.py b/slayer/storage/yaml_storage.py index 33dc952d..0a60c720 100644 --- a/slayer/storage/yaml_storage.py +++ b/slayer/storage/yaml_storage.py @@ -407,7 +407,8 @@ async def update_column_sampled( # ---- datasource CRUD --------------------------------------------------- - async def _save_datasource_impl(self, datasource: DatasourceConfig) -> None: + async def save_datasource(self, datasource: DatasourceConfig) -> None: + await self.check_datasource_id_collision(datasource.name) path = os.path.join(self.datasources_dir, f"{datasource.name}.yaml") data = datasource.model_dump(mode="json", exclude_none=True) with open(path, "w") as f: From 1fd035386427155bc3ccb456bc4b422b1175757e Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 22 Jul 2026 18:14:12 +0400 Subject: [PATCH 3/5] Trim docstrings and comments to the essentials Claude-Session: https://claude.ai/code/session_011sbQKNXcUtGAsCXtmJXHvR --- slayer/core/errors.py | 16 ++++-------- slayer/mcp/server.py | 3 +-- slayer/storage/base.py | 45 +++++++++----------------------- slayer/storage/join_sync.py | 4 --- slayer/storage/sqlite_storage.py | 5 ++-- slayer/storage/v4_migration.py | 14 ++++------ slayer/storage/yaml_storage.py | 28 +++++++------------- tests/test_id_collision.py | 30 +++++---------------- tests/test_memory_string_ids.py | 4 --- tests/test_yaml_exact_entry.py | 11 +++----- 10 files changed, 45 insertions(+), 115 deletions(-) diff --git a/slayer/core/errors.py b/slayer/core/errors.py index ae54582e..75a7c1b3 100644 --- a/slayer/core/errors.py +++ b/slayer/core/errors.py @@ -118,17 +118,11 @@ def __init__(self, cycle: list[tuple[str, str]]) -> None: class IdCollisionError(SlayerError, ValueError): """Raised when saving an entity whose id differs from an existing id - only by letter case (``Orders`` vs ``orders``). - - Ids become filenames in the YAML backend, where case-variant ids - address the same file on case-insensitive filesystems (macOS, - Windows). The check runs uniformly on every backend so stores stay - portable across backends and platforms. - - ``kind`` is one of ``"model"`` / ``"datasource"`` / ``"memory"``; - ``data_source`` is set for model-name collisions. Multi-inherits - ``ValueError`` so existing ``except ValueError`` call sites continue - to work unchanged. + only by letter case — such ids collide as filenames in the YAML + backend on case-insensitive filesystems, so every backend rejects + them. ``kind`` is ``"model"`` / ``"datasource"`` / ``"memory"``. + Multi-inherits ``ValueError`` so existing ``except ValueError`` call + sites continue to work unchanged. """ _LABELS = { diff --git a/slayer/mcp/server.py b/slayer/mcp/server.py index ea5c4d28..1fa09ee6 100644 --- a/slayer/mcp/server.py +++ b/slayer/mcp/server.py @@ -1374,8 +1374,7 @@ async def create_datasource( await storage.save_model(model) saved_models.append(model) except ValueError as exc: - # e.g. quoted case-variant tables ("Orders" vs orders) — - # report and continue instead of aborting the whole ingest. + # e.g. quoted case-variant tables — report and continue. save_errors.append(f"- {model.name}: {exc}") models = saved_models diff --git a/slayer/storage/base.py b/slayer/storage/base.py index 635e4a04..12757e67 100644 --- a/slayer/storage/base.py +++ b/slayer/storage/base.py @@ -118,8 +118,7 @@ def _entity_matches_cascade( def _fs_equivalence_key(value: str) -> str: - """Identity key under which two ids would address the same file on a - case-insensitive filesystem (macOS / Windows defaults).""" + """Key under which two ids collide on a case-insensitive filesystem.""" return value.casefold() @@ -127,12 +126,9 @@ def _find_case_colliding_id( candidate: str, existing: Iterable[str], ) -> str | None: """Return an existing id that casefold-equals ``candidate`` but is - spelled differently, or ``None``. - - An exact match never counts — same-id saves are upserts. A - raw-different collider is reported even when an exact match also - exists, so a legacy store holding both spellings surfaces the pair on - a save of either one. + spelled differently, or ``None``. An exact match never counts + (upserts); a collider is reported even alongside an exact match so a + legacy store holding both spellings surfaces the pair. """ key = _fs_equivalence_key(candidate) for entry in existing: @@ -221,15 +217,9 @@ async def save_model( await self._save_model_impl(model) async def _check_model_identity_collision(self, model: SlayerModel) -> None: - """Reject a model whose identity differs only by case from an - existing one — such ids alias to the same file in the YAML backend - on case-insensitive filesystems, so they are rejected uniformly. - - Checks both path components: ``data_source`` against registered - datasource names and other models' data_sources (they share the - ``models//`` directory namespace), and ``name`` against models - in the same datasource. - """ + """Reject a model whose ``data_source`` or ``name`` differs only + by case from an existing one — both are filename components in + the YAML backend.""" identities = await self._list_all_model_identities() known_ds = {ds for ds, _ in identities} known_ds.update(await self.list_datasources()) @@ -453,22 +443,14 @@ async def _migrate_and_refine_on_load( @abstractmethod async def save_datasource(self, datasource: DatasourceConfig) -> None: """Persist a datasource config (upsert by exact name). - Implementations should call :meth:`check_datasource_id_collision` - with ``datasource.name`` before writing so case-variant names are - rejected — the built-in YAML and SQLite backends do. - """ + before writing.""" async def check_datasource_id_collision(self, name: str) -> None: """Raise :class:`IdCollisionError` when ``name`` differs only by - case from an existing datasource name or from a saved model's - ``data_source`` (they share the ``models//`` directory - namespace in the YAML backend). An exact match is fine — saves - are upserts. - - Public so custom backends can opt into the same protection from - their own ``save_datasource`` implementations. - """ + case from an existing datasource name or a saved model's + ``data_source``. Public so custom backends can call it from + ``save_datasource``.""" existing = set(await self.list_datasources()) existing.update(ds for ds, _ in await self._list_all_model_identities()) collide = _find_case_colliding_id(name, existing) @@ -674,9 +656,8 @@ async def _next_memory_seq(self) -> str: ``"001"`` are ignored. Empty corpus → ``"1"``.""" async def _list_memory_ids(self) -> list[str]: - """Every persisted memory id. The default loads full rows; - backends override with a cheaper id-only listing (directory scan, - ``SELECT id``).""" + """Every persisted memory id; backends override with a cheap + id-only listing.""" return [m.id for m in await self._list_memories_rows(entities=None)] async def save_memory( diff --git a/slayer/storage/join_sync.py b/slayer/storage/join_sync.py index 24c6c2f2..a34f4bf4 100644 --- a/slayer/storage/join_sync.py +++ b/slayer/storage/join_sync.py @@ -231,8 +231,6 @@ async def update_column_sampled( ) async def save_datasource(self, datasource: DatasourceConfig) -> None: - # Pure delegation — the case-collision check runs once, inside the - # inner backend's implementation. return await self._inner.save_datasource(datasource) async def get_datasource(self, name: str) -> DatasourceConfig | None: @@ -270,8 +268,6 @@ async def _next_memory_seq(self) -> str: return await self._inner._next_memory_seq() async def _list_memory_ids(self) -> list[str]: - # The base save_memory template runs at this wrapper level; route - # its id listing to the inner backend's cheap override. return await self._inner._list_memory_ids() async def strip_dangling_entities_from_memories( diff --git a/slayer/storage/sqlite_storage.py b/slayer/storage/sqlite_storage.py index f7f530f7..6e3af73f 100644 --- a/slayer/storage/sqlite_storage.py +++ b/slayer/storage/sqlite_storage.py @@ -437,9 +437,8 @@ def _save_memory_atomic_sync( if memory_id is None: memory_id = self._next_memory_seq_sync_from_conn(conn) else: - # Case-collision check must run inside this transaction: - # this override bypasses the base save_memory template - # where the shared check lives. + # Duplicated here because this override bypasses the + # base save_memory template. ids = [ row[0] if isinstance(row[0], str) else str(row[0]) for row in conn.execute("SELECT id FROM memories") diff --git a/slayer/storage/v4_migration.py b/slayer/storage/v4_migration.py index 52ad1921..2bdd061c 100644 --- a/slayer/storage/v4_migration.py +++ b/slayer/storage/v4_migration.py @@ -102,12 +102,9 @@ def _check_layout_case_collisions( models_dir: str, planned: list[tuple[str, dict, str, str]], ) -> None: - """Refuse the layout migration when two targets differ only by case: - on a case-insensitive filesystem the second write would clobber the - first. Planned targets are checked against each other and against the - existing v4 subdirectories/files, before anything moves — a failure - leaves every flat file in place. - """ + """Refuse the migration when two targets differ only by case (the + second write would clobber the first). Checks planned targets against + each other and against existing v4 entries, before anything moves.""" ds_by_key: dict[str, str] = {} file_by_key: dict[tuple[str, str], str] = {} for entry in os.listdir(models_dir): @@ -162,9 +159,8 @@ def migrate_yaml_layout(base_dir: str) -> None: available = _yaml_list_datasource_names(datasources_dir) - # Pass 1: read every flat file and resolve its datasource without - # moving anything, so the collision check below can veto the whole - # migration while the flat files are still intact. + # Pass 1: resolve datasources without moving anything, so the + # collision check can veto the migration with flat files intact. planned: list[tuple[str, dict, str, str]] = [] for filename in flat_files: path = os.path.join(models_dir, filename) diff --git a/slayer/storage/yaml_storage.py b/slayer/storage/yaml_storage.py index 0a60c720..beb4ff85 100644 --- a/slayer/storage/yaml_storage.py +++ b/slayer/storage/yaml_storage.py @@ -106,13 +106,9 @@ def _atomic_write_text(path: str, text: str) -> None: def _exact_entry_exists(dir_path: str, entry_name: str) -> bool: - """True iff ``dir_path`` contains an entry named exactly ``entry_name``. - - ``os.path.exists`` matches any case variant on a case-insensitive - filesystem, so ``get_model("Orders")`` would silently open - ``orders.yaml``. Comparing against ``os.listdir`` restores exact-id - semantics: a case mismatch reads as "not found". - """ + """True iff ``dir_path`` contains an entry named exactly + ``entry_name`` — unlike ``os.path.exists``, which matches any case + variant on a case-insensitive filesystem.""" try: return entry_name in os.listdir(dir_path) except (FileNotFoundError, NotADirectoryError): @@ -198,11 +194,8 @@ def migrate_memories_layout(base_dir: str) -> None: ) normalized = _normalize_legacy_memory_rows(rows) mem_dir = os.path.join(base_dir, "memories") - # Reject ids that differ only by case BEFORE writing anything: on a - # case-insensitive filesystem the second ``.md`` write would clobber - # the first and the legacy file would then be deleted — data loss. - # Stems already in memories/ participate too, so a crash-resumed run - # can't clobber a file written by a previous attempt. + # Case-colliding ids would clobber each other as .md files; refuse + # before writing anything. Existing stems included for crash-resume. id_by_key: dict[str, str] = {} if os.path.isdir(mem_dir): for fname in os.listdir(mem_dir): @@ -302,8 +295,7 @@ def _model_path(self, data_source: str, name: str) -> str: return os.path.join(self.models_dir, data_source, f"{name}.yaml") def _model_entry_exists(self, data_source: str, name: str) -> bool: - """Exact-case existence check for both path components of a model - (the datasource directory and the model file).""" + """Exact-case existence check for both path components of a model.""" return _exact_entry_exists( self.models_dir, data_source, ) and _exact_entry_exists( @@ -362,8 +354,7 @@ async def get_model( async def _delete_model_row( self, *, data_source: str, name: str, ) -> bool: - # Exact match required: os.remove on a case-insensitive filesystem - # would otherwise delete a case-variant sibling's file. + # Exact match — os.remove would otherwise hit a case-variant sibling. if not self._model_entry_exists(data_source, name): return False os.remove(self._model_path(data_source, name)) @@ -578,9 +569,8 @@ async def _get_memory_row(self, memory_id: str) -> Memory | None: # the check and the open surfaces as FileNotFoundError → treat as # "missing" (return None) rather than crash. path = self._memory_md_path(memory_id) - # The .md content carries no id (the filename is the identity), so - # an exact listdir check is the only way to keep a case-variant - # lookup from opening the wrong file on a case-insensitive FS. + # Filename IS the identity; exact check keeps a case-variant + # lookup from opening the wrong file. if not _exact_entry_exists(self._memories_dir, f"{memory_id}.md"): return None try: diff --git a/tests/test_id_collision.py b/tests/test_id_collision.py index 52ec0e11..2dd9306f 100644 --- a/tests/test_id_collision.py +++ b/tests/test_id_collision.py @@ -1,10 +1,7 @@ """Case-collision rejection for datasource / model / memory ids. -Ids become filenames in the YAML backend, where ids differing only by -case address the same file on case-insensitive filesystems (macOS / -Windows defaults). Saves of a case-variant id are rejected uniformly on -every backend so stores stay portable across backends and platforms. -Exact-id re-saves remain upserts. +Saving an id that differs only by case from an existing one raises +IdCollisionError on every backend; exact-id re-saves remain upserts. """ from __future__ import annotations @@ -65,8 +62,6 @@ def test_case_variant_found(self) -> None: assert _find_case_colliding_id("Orders", ["orders"]) == "orders" def test_variant_reported_even_with_exact_match(self) -> None: - # A legacy store holding both spellings must surface the pair on - # a save of either one. assert _find_case_colliding_id("orders", ["orders", "Orders"]) == "Orders" def test_unrelated_ids_do_not_collide(self) -> None: @@ -98,12 +93,10 @@ async def test_exact_resave_upserts(self, storage: StorageBackend) -> None: async def test_collides_with_model_data_source( self, storage: StorageBackend, ) -> None: - # Models saved under an orphan datasource reserve its name: on - # YAML they share the models// directory namespace. + # Models under an orphan datasource reserve its name. await storage.save_model(_model("orders", data_source="db")) with pytest.raises(IdCollisionError): await storage.save_datasource(_ds("DB")) - # An exactly-named config for the orphan datasource is fine. await storage.save_datasource(_ds("db")) @@ -135,8 +128,6 @@ async def test_exact_resave_upserts(self, storage: StorageBackend) -> None: async def test_same_name_in_other_datasource_ok( self, storage: StorageBackend, ) -> None: - # The name check is scoped to one datasource; case variants across - # datasources live in different directories. await storage.save_model(_model("orders", data_source="db_a")) await storage.save_model(_model("Orders", data_source="db_b")) assert await storage.get_model("orders", data_source="db_a") is not None @@ -160,15 +151,13 @@ async def test_data_source_vs_other_models_rejected( async def test_validate_false_bypasses( self, storage: StorageBackend, ) -> None: - # Trusted internal callers (the migration write-back) must stay - # able to re-persist legacy data that predates the check. + # The migration write-back path must stay able to persist legacy data. await storage.save_model(_model("orders")) await storage.save_model(_model("Orders"), _validate=False) # --------------------------------------------------------------------------- -# Memories (the save-time rejection itself is covered per-backend in -# test_memory_string_ids.py::test_case_colliding_ids_rejected) +# Memories (rejection also covered in test_memory_string_ids.py) # --------------------------------------------------------------------------- @@ -196,8 +185,7 @@ async def test_error_attrs(self, storage: StorageBackend) -> None: # --------------------------------------------------------------------------- -# Legacy stores that already hold a colliding pair (created pre-check, on -# a case-sensitive filesystem): a save of EITHER spelling must raise. +# Legacy stores already holding a colliding pair: either save must raise # --------------------------------------------------------------------------- @@ -218,7 +206,6 @@ async def test_sqlite_memory_pair_blocks_both(self) -> None: await storage.save_memory( id=mid, learning="update", entities=[], ) - # Both rows are still intact. assert (await storage.get_memory("X")).learning == "upper" assert (await storage.get_memory("x")).learning == "lower" @@ -240,8 +227,7 @@ async def test_sqlite_model_pair_blocks_both(self) -> None: # --------------------------------------------------------------------------- -# The production wrapper (resolve_storage → JoinSyncStorage) propagates -# the rejection through every save surface. +# The production wrapper (resolve_storage → JoinSyncStorage) # --------------------------------------------------------------------------- @@ -283,7 +269,6 @@ def test_legacy_memories_yaml_case_pair_refused(self) -> None: ) with pytest.raises(ValueError, match="differ only by"): YAMLStorage(base_dir=tmpdir) - # Legacy file preserved; no .md written. assert os.path.exists(legacy) mem_dir = os.path.join(tmpdir, "memories") md_files = ( @@ -305,7 +290,6 @@ def test_flat_models_case_variant_datasources_refused(self) -> None: ) with pytest.raises(ValueError, match="differs only by case"): YAMLStorage(base_dir=tmpdir) - # Flat files untouched; no subdirectory created. assert sorted(os.listdir(models_dir)) == ["a.yaml", "b.yaml"] def test_flat_model_vs_existing_v4_case_variant_refused(self) -> None: diff --git a/tests/test_memory_string_ids.py b/tests/test_memory_string_ids.py index 1e6d3fd9..056295a1 100644 --- a/tests/test_memory_string_ids.py +++ b/tests/test_memory_string_ids.py @@ -183,9 +183,6 @@ async def test_save_with_user_id(self, storage: StorageBackend) -> None: async def test_case_colliding_ids_rejected( self, storage: StorageBackend, ) -> None: - # Ids differing only by case would alias to the same file in the - # YAML backend on case-insensitive filesystems; rejected uniformly - # on every backend so stores stay portable. await storage.save_memory( id="X", learning="upper", entities=["mydb.orders"], ) @@ -195,7 +192,6 @@ async def test_case_colliding_ids_rejected( ) assert exc_info.value.new_id == "x" assert exc_info.value.existing_id == "X" - # The original memory is untouched. upper = await storage.get_memory("X") assert upper.learning == "upper" diff --git a/tests/test_yaml_exact_entry.py b/tests/test_yaml_exact_entry.py index 7861d28a..efc2190c 100644 --- a/tests/test_yaml_exact_entry.py +++ b/tests/test_yaml_exact_entry.py @@ -1,11 +1,6 @@ -"""YAML read/delete exact-name verification. - -On a case-insensitive filesystem ``open`` / ``os.path.exists`` match any -case variant of a filename, so a lookup for ``Orders`` would silently hit -``orders.yaml`` — and a delete would remove it. The YAML backend compares -against ``os.listdir`` instead, so a case mismatch behaves as "not -found". These assertions hold on both case-sensitive and -case-insensitive dev machines. +"""YAML read/delete exact-name verification: a case-variant lookup +behaves as "not found" instead of hitting the wrong file on a +case-insensitive filesystem. Assertions hold on both filesystem kinds. """ from __future__ import annotations From 4bf77dc52346a3b8d53ab4b91e31df9174215429 Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 22 Jul 2026 18:51:08 +0400 Subject: [PATCH 4/5] Scope case-collision rejection to the YAML backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SQLite keys are case-sensitive TEXT PKs and handle case-variant ids fine; enforcing the rule there added complexity (a duplicated transaction check, the _list_memory_ids primitive with per-backend overrides) for no real protection. Revert SQLite to its original behavior: case-variant ids are distinct rows. The checks stay in the base save templates — preserving the _validate=False migration bypass and coverage through JoinSyncStorage — gated on a class flag, _ids_collide_as_filenames (True on YAMLStorage, copied from the inner backend by the wrapper). Custom backends with a case-insensitive keyspace can opt in via the same flag plus check_datasource_id_collision. Claude-Session: https://claude.ai/code/session_011sbQKNXcUtGAsCXtmJXHvR --- CLAUDE.md | 2 +- docs/concepts/ingestion.md | 2 +- docs/concepts/memories.md | 9 ++- docs/concepts/models.md | 2 +- docs/configuration/storage.md | 13 ++-- slayer/core/errors.py | 12 ++-- slayer/storage/base.py | 43 ++++++----- slayer/storage/join_sync.py | 4 +- slayer/storage/sqlite_storage.py | 24 ------- slayer/storage/yaml_storage.py | 5 +- tests/test_id_collision.py | 120 +++++++++++++++---------------- tests/test_memory_string_ids.py | 18 ++--- 12 files changed, 112 insertions(+), 142 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1021b5dc..7d5a17b9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,7 +53,7 @@ poetry run ruff check slayer/ tests/ - **Schema versioning**: `SlayerModel`, `SlayerQuery`, and `DatasourceConfig` carry a `version: int` (currently `7` for `SlayerModel`, `3` for `SlayerQuery`, `1` for `DatasourceConfig`). On load, older versions are upgraded via the converter chain in `slayer/storage/migrations.py` before Pydantic validates the dict. Saves always emit the current version. The hook is on the Pydantic class itself (`@model_validator(mode="before")`), so every storage backend — YAML, SQLite, third-party backends, plus MCP/API/dbt entry points — gets migrations automatically without backend changes. The v1→v2 converter (in `slayer/storage/v2_migration.py`) merges v1 `dimensions`+`measures` into v2 `columns`, repurposes `measures` to hold `ModelMeasure` formulas, and renames `SlayerQuery.fields`→`measures`. The v2→v3 converter (in `slayer/storage/v3_migration.py`) drops the legacy `dry_run`/`explain` fields from `SlayerQuery` (they are now engine kwargs only — `engine.execute(query, dry_run=..., explain=...)`) and walks `SlayerModel.source_queries` entries through the SlayerQuery chain. The v3→v4 converter (in `slayer/storage/v4_migration.py`) requires non-empty `data_source` on table-backed SlayerModel dicts (`sql_table` or `sql` mode); query-backed models (`source_queries` set) are exempt because their `data_source` is filled by `engine._validate_and_populate_cache` from the resolved virtual model before save. The v4 converter also ships layout migrators that move legacy `models/.yaml` flat files into `models//.yaml` and rebuild the SQLite `models` table with a composite `(data_source, name)` primary key. The v4→v5 converter (in `slayer/storage/v5_migration.py`, DEV-1361) coarse-renames `Column.type` legacy values to the new sqlglot-aligned vocabulary (`string→TEXT`, `number→DOUBLE`, `time→TIMESTAMP`, etc.) and strips the dead aggregation pseudo-types (`count`/`sum`/...). The v5→v6 converter (in `slayer/storage/v6_migration.py`, DEV-1375) is a no-op forward — v6 introduces a single new optional field, `Column.sampled: Optional[str]`, that caches the per-column sample-value snapshot consumed by [`search`](docs/concepts/search.md) and `inspect_model`. The v6→v7 converter (in `slayer/storage/v7_migration.py`, DEV-1480) is also a no-op forward — v7 adds two more `Column` fields: `sampled_values: Optional[List[str]]` (the structured top-50-by-frequency list, paired with the existing text `sampled`) and `distinct_count: Optional[int]` (true cardinality at profile time, surfaced via a secondary `count_distinct` query on overflow). Storage backends additionally introspect each model's datasource on first load and refine `DOUBLE → INT` for base columns whose live SQL type is integer (`slayer/storage/type_refinement.py`); the refined model is written back so subsequent loads skip both steps. `SlayerQuery` v3 sets `extra="forbid"` so unknown fields raise. See [docs/concepts/models.md](docs/concepts/models.md#schema-versioning). - **DataType / CAST emission** (v5, DEV-1361): `DataType` values match sqlglot's `exp.DataType.Type` byte-for-byte: `TEXT`, `INT`, `DOUBLE`, `BOOLEAN`, `DATE`, `TIMESTAMP`. Auto-ingestion narrows integer DB types (INTEGER/BIGINT/SERIAL/INT8…/UINT8…) to `INT`, and floats/numerics (FLOAT/DOUBLE/DECIMAL with scale>0) to `DOUBLE`; NUMERIC(p,0) and DECIMAL(p,0) are integer-shaped and narrow to `INT`. The SQL generator wraps **non-bare** `Column.sql` (function calls, arithmetic, CASE WHEN) in `CAST(... AS )` driven by `Column.type`; bare identifiers and `sql=None` are emitted unchanged. `TEXT` is a no-op (skipped, since `CAST AS TEXT` is cosmetic and doesn't unwrap SQLite's JSON-quoted strings anyway). `ModelMeasure.type` (also `Optional[DataType]`) declares the formula's result type; when set, the aggregation expression is wrapped in an outer CAST. Lenient `before`-validators on `Column.type` and `ModelMeasure.type` absorb legacy lowercase agent input (`"string"` → `TEXT`, `"number"` → `DOUBLE`, etc.) and silently drop dropped pseudo-types. `slayer storage migrate-types --dry-run [--data-source X]` runs the storage refinement step explicitly for batch / inspectable usage. Schema-drift detection (`data_type_bucket`) keeps `INT` and `DOUBLE` in the same `"number"` bucket so a `DOUBLE`-typed persisted column does not flag as drift against an `INT` live column. - **Datasource-scoped storage** (v4, DEV-1330): Models are keyed by `(data_source, name)`, not bare `name`. Two datasources can share a table name without collision. `storage.get_model(name, data_source=None)` and `storage.delete_model(name, data_source=None)` resolve bare names by: (1) returning the unique match if exactly one model has that name, (2) walking `storage.get_datasource_priority()` to pick the first datasource in the priority list that has the name, (3) raising `slayer.core.errors.AmbiguousModelError` otherwise. The priority list is configured via `storage.set_datasource_priority(["db_a", ...])`, the MCP `set_datasource_priority` tool, the REST `PUT /datasources/priority`, or the `slayer datasources priority` CLI subcommand. `engine.execute(query, data_source=...)` passes a hint that wins over the priority list. Joins resolve targets within the parent model's `data_source` only — cross-datasource joins are not auto-mirrored. A sibling Linear issue ([DEV-1342](https://linear.app/motley-ai/issue/DEV-1342)) tracks whether to add `datasource.model_name` dot syntax inside query strings. -- **Case-collision rejection** (issue #249): saving a datasource, model, or memory whose id differs only by letter case (`casefold()`-equal, raw-different) from an existing one raises `slayer.core.errors.IdCollisionError(SlayerError, ValueError)` — attrs `kind`/`new_id`/`existing_id`/`data_source`. Ids become filenames in the YAML backend (case variants alias on macOS/Windows); the check is enforced in the `StorageBackend` base templates so EVERY backend rejects on EVERY platform and stores stay portable. Exact-id re-saves remain upserts; a legacy store already holding a colliding pair raises on a save of either spelling. Model saves check both components: `data_source` against registered datasource names ∪ other models' data_sources (shared `models//` dir namespace), and `name` within the same datasource. `save_datasource` stays a backend-implemented abstract method (no rename — third-party overrides keep working); the datasource check lives in the public base helper `check_datasource_id_collision(name)`, which the built-in YAML/SQLite implementations call before writing (custom backends opt in the same way). `save_memory` checks via the cheap `_list_memory_ids()` primitive (YAML: listdir; SQLite: `SELECT id`; JoinSync: delegation). `SQLiteStorage.save_memory` bypasses the base template, so the same check is duplicated inside `_save_memory_atomic_sync`'s transaction. `save_model(_validate=False)` (migration write-back) bypasses the check. YAML reads/deletes verify the EXACT directory entry via `os.listdir` comparison (`_exact_entry_exists`) — a case-variant `get_model`/`get_datasource`/`get_memory` returns not-found and a delete is a no-op instead of silently hitting the wrong file. `migrate_memories_layout` and `migrate_yaml_layout` pre-check collisions before writing anything (legacy files preserved on failure). Surfacing: REST `POST /datasources` → 400 (model/memory endpoints already mapped ValueError→400); MCP `create_datasource` returns a friendly message and its auto-ingest loop reports per-model save failures without aborting; CLI prints and exits (or skips-and-continues in ingest loops). +- **Case-collision rejection, YAML-only** (issue #249): saving a datasource, model, or memory whose id differs only by letter case (`casefold()`-equal, raw-different) from an existing one raises `slayer.core.errors.IdCollisionError(SlayerError, ValueError)` — attrs `kind`/`new_id`/`existing_id`/`data_source` — in the **YAML backend only** (ids are filenames there; case variants alias on macOS/Windows). SQLite deliberately keeps case-variant ids as distinct rows (case-sensitive TEXT PKs) — which means a SQLite store holding such a pair can't be exported to the YAML layout without renaming. Mechanism: the checks live in the `StorageBackend` base templates (`save_model` under `_validate`, `save_memory` for user-supplied ids) gated on the class flag `_ids_collide_as_filenames` (False on the base, True on `YAMLStorage`, copied from `inner` by `JoinSyncStorage.__init__` so wrapped YAML still rejects); this placement preserves the `save_model(_validate=False)` migration-write-back bypass. The datasource check is the public base helper `check_datasource_id_collision(name)` (checks datasource names ∪ model data_sources — shared `models//` dir namespace) called by `YAMLStorage.save_datasource`; `save_datasource` itself stays a backend-implemented abstract method. Custom backends with a case-insensitive keyspace opt in via the flag + helper. Exact-id re-saves remain upserts; model saves check `data_source` and `name` (within the same datasource). YAML reads/deletes verify the EXACT directory entry via `os.listdir` comparison (`_exact_entry_exists`) — a case-variant `get_model`/`get_datasource`/`get_memory` returns not-found and a delete is a no-op instead of silently hitting the wrong file. `migrate_memories_layout` and `migrate_yaml_layout` pre-check collisions before writing anything (legacy files preserved on failure). Surfacing: REST `POST /datasources` → 400 (model/memory endpoints already mapped ValueError→400); MCP `create_datasource` returns a friendly message and its auto-ingest loop reports per-model save failures without aborting; CLI prints and exits (or skips-and-continues in ingest loops). - **Source modes**: a `SlayerModel` has exactly one source: `sql_table` (physical table), `sql` (explicit SQL subquery), or `source_queries` (query-backed: `List[SlayerQuery]`). Validators enforce mutual exclusivity, reject empty `source_queries=[]`, require `name` on every non-final stage, and reject duplicate stage names. `SlayerModel.source_queries` entries given as dicts are auto-parsed into `SlayerQuery` instances by a Pydantic before-validator. - **Stages form a DAG, not just a chain**: any stage in `source_queries` or a runtime query list may use a named sibling as `source_model` or as `joins.target_model`. The **runtime list path** (`engine.execute(query=[...])`, CLI `slayer query @file.json`, MCP `query_nested`) **auto-sorts** the input via `SlayerQueryEngine._topologically_order_queries` — a hand-rolled Kahn's algorithm (no `graphlib` dep) that reorders so every stage appears after the siblings it references. The last entry of the input stays last as the entry point / DAG root, so the result returned doesn't depend on sort order. Validations performed up front: missing `name` on a non-final entry; duplicate names; self-references; the root being referenced by any other stage (root must be the dependency sink); cycles. Unreachable utility sub-queries are accepted — they flow through the sort but are silently dropped from the emitted SQL since nothing resolves them. Stored `SlayerModel.source_queries` retains strict-order semantics (relies on `_scope_named_queries_to_prior` plus the `_forbidden_sibling_refs_var` ContextVar in `_resolve_model_inner` / `_resolve_join_target`) — YAML-defined source_queries are read top-to-bottom and the strict-order error pins typos at resolve time. The runtime list's reference extractor walks `source_model` (string sibling refs), `ModelExtension.source_name`, and `joins[].target_model` from both dict and typed shapes; formula-only references resolve correctly because the explicit join they require is what the extractor picks up. - **Query-backed models**: `SlayerModel.query_variables: Dict[str, Any]` provides defaults for `{var}` placeholders in `source_queries`. `engine.create_model_from_query(query, name, variables=..., save=True)` saves a query as a query-backed model and populates the `columns` + `backing_query_sql` cache. `engine.save_model(model)` is the engine-side save helper that runs source-mode validation + cache refresh + persistence; user-supplied `columns` and `backing_query_sql` on a query-backed model are rejected at save with a clear error. The cache is refreshed only on save paths (`engine.save_model` / `create_model_from_query(save=True)`); `engine.execute` never writes to storage, even on stale or empty caches (closes #74 — `tests/test_query_backed_models.py::test_execute_never_writes_to_storage` pins this). diff --git a/docs/concepts/ingestion.md b/docs/concepts/ingestion.md index 2d2e6bbd..09cff708 100644 --- a/docs/concepts/ingestion.md +++ b/docs/concepts/ingestion.md @@ -276,7 +276,7 @@ Ingest-on-startup: N/M datasources ingested (K failed: name1, name2) - **Existing `sql_table`-mode model** → append new columns and joins from the live schema. Existing columns and joins are **never** mutated — `description`, `label`, `format`, `meta`, and `allowed_aggregations` are preserved verbatim. - **Existing `sql`-mode or query-backed model with the matching name** → skipped silently; those are user-authored. -Two live tables whose quoted names differ only by letter case (`"Orders"` vs `orders`) cannot both be persisted — model names collide as filenames in YAML storage on macOS / Windows, so the save is rejected uniformly (`IdCollisionError`). The first table wins; the second surfaces as a per-model entry in `IdempotentIngestResult.errors` (or a per-model message on the CLI / MCP paths) without aborting the rest of the ingest. +With the default YAML storage, two live tables whose quoted names differ only by letter case (`"Orders"` vs `orders`) cannot both be persisted — model names collide as filenames on macOS / Windows, so the save is rejected (`IdCollisionError`). The first table wins; the second surfaces as a per-model entry in `IdempotentIngestResult.errors` (or a per-model message on the CLI / MCP paths) without aborting the rest of the ingest. SQLite storage persists both. After the additive pass, `validate_models` runs against the in-scope models and the result is merged into the response (`IdempotentIngestResult.to_delete`). Type-bucket drift on existing columns surfaces there — apply via `slayer validate-models --force-clean`, then re-ingest to pick up the new live type. See [Schema Drift](schema-drift.md) for the full diff / cascade contract. diff --git a/docs/concepts/memories.md b/docs/concepts/memories.md index 34674ef3..437e8070 100644 --- a/docs/concepts/memories.md +++ b/docs/concepts/memories.md @@ -77,11 +77,10 @@ int-shaped id (`"1"`, `"2"`, ...). Supply a string for a stable user-controlled id (`"kb.policy.42"`) — useful for knowledge-base ingestion pipelines. Charset excludes `:`, `/`, `?`, `#`, whitespace, and ASCII control characters. Duplicate id → unconditional **upsert**, -`created_at` preserved. An id that differs only by letter case from an -existing one (`X` vs `x`) raises `IdCollisionError` — ids become -filenames in the YAML backend, where case variants collide on -macOS / Windows, and the rule is enforced on every backend so stores -stay portable. +`created_at` preserved. In the default YAML storage, an id that differs +only by letter case from an existing one (`X` vs `x`) raises +`IdCollisionError` — ids are filenames there, and case variants collide +on macOS / Windows. `description` is optional (≤ 500 chars). When set, `search(compact=True)` and `inspect_model(compact=True)` surface this short preview instead of diff --git a/docs/concepts/models.md b/docs/concepts/models.md index 81b135f6..6831a6cc 100644 --- a/docs/concepts/models.md +++ b/docs/concepts/models.md @@ -22,7 +22,7 @@ A query then asks for `revenue:sum` (aggregate the `revenue` column), `aov` (the | Field | Type | Required | Description | |-------|------|----------|-------------| -| `name` | string | Yes | Unique model name. Names differing only by letter case (`Orders` vs `orders`) are rejected at save time (`IdCollisionError`) — they would collide as filenames in YAML storage on macOS / Windows | +| `name` | string | Yes | Unique model name. In the default YAML storage, names differing only by letter case (`Orders` vs `orders`) are rejected at save time (`IdCollisionError`) — they would collide as filenames on macOS / Windows | | `sql_table` | string | One of | A physical database table (e.g. `public.orders`) | | `sql` | string | these | A SQL subquery to use as the source | | `source_queries` | list[SlayerQuery] | three | Saved query stages — makes the model **query-backed** | diff --git a/docs/configuration/storage.md b/docs/configuration/storage.md index 1aad00cc..1b478557 100644 --- a/docs/configuration/storage.md +++ b/docs/configuration/storage.md @@ -64,7 +64,7 @@ slayer_data/ **Layout note:** Models live under `models//.yaml` so two datasources sharing a table name don't collide. Opening a `YAMLStorage` on a legacy flat directory migrates `models/.yaml` files into the nested layout automatically. If a flat file has an empty `data_source` and exactly one datasource is registered, the migrator auto-fills it; otherwise it hard-fails so the user can edit `data_source` by hand before reopening. -**Name collisions:** Because names become filenames here, two ids differing only by letter case (`Orders` vs `orders`) would address the same file on macOS / Windows. Saving a datasource, model, or memory whose id differs only by case from an existing one therefore raises `IdCollisionError` — on **every** backend and platform, so a store created on Linux stays portable. Re-saving the exact same id is still a normal upsert. On the read side, YAML lookups compare the exact filename, so `get_model("Orders")` when only `orders.yaml` exists returns "not found" instead of the wrong model (and a delete is a no-op). The layout migrations apply the same check up front and refuse to run — legacy files untouched — if migrating would collide. +**Name collisions:** Because names become filenames here, two ids differing only by letter case (`Orders` vs `orders`) would address the same file on macOS / Windows. Saving a datasource, model, or memory whose id differs only by case from an existing one therefore raises `IdCollisionError` — on every platform, so a YAML store created on Linux stays portable. Re-saving the exact same id is still a normal upsert. On the read side, lookups compare the exact filename, so `get_model("Orders")` when only `orders.yaml` exists returns "not found" instead of the wrong model (and a delete is a no-op). The layout migrations apply the same check up front and refuse to run — legacy files untouched — if migrating would collide. This is YAML-specific: SQLite keys are case-sensitive and store case-variant ids distinctly (which also means such a SQLite store cannot be exported to the YAML layout without renaming). **Embeddings sidecar (DEV-1405):** Embedding rows used by the optional dense-search channel live in a SQLite file at `/embeddings.db`, **not** in `embeddings.yaml`. Embeddings are derived artifacts (regeneratable by `slayer ingest` / `--ingest-on-startup`), not user-authored config, so the diffable-in-git property that drives the YAML choice for models doesn't apply. A pre-DEV-1405 `embeddings.yaml` or `counters.yaml` is silently renamed to `.yaml.legacy` on first open and ignored thereafter; re-run `slayer ingest` to repopulate `embeddings.db`. The schema is identical to the `SQLiteStorage` embedding table — both backends delegate to a shared `SidecarEmbeddingStore` helper. @@ -82,8 +82,6 @@ storage = SQLiteStorage(db_path="./slayer.db") Tables: `models`, `datasources`, `settings` (for the datasource priority list), `memories` + `memory_entities` (indexed by canonical entity), and `embeddings` (cached embedding rows keyed by `(canonical_id, embedding_model_name)`). Memory ids are assigned by SQLite's `INTEGER PRIMARY KEY` rowid mechanism inside the save transaction — no separate counter table is needed. -SQLite keys are case-sensitive, but the case-collision rule above applies here too (enforced in the shared base class) so a SQLite store can always be exported to the YAML layout and moved across platforms. - ## Storage Resolution The `resolve_storage()` factory creates a backend from a path or URI: @@ -114,8 +112,8 @@ from slayer.core.models import SlayerModel, DatasourceConfig class MyCustomStorage(StorageBackend): # Models are keyed by (data_source, name). ``save_model`` is a template - # method on the base class (it runs shared validation — cycle detection, - # case-collision rejection); backends implement only ``_save_model_impl``. + # method on the base class (it runs shared validation); backends + # implement only ``_save_model_impl``. def _save_model_impl(self, model: SlayerModel) -> None: ... def _list_all_model_identities(self) -> list[tuple[str, str]]: ... def get_model(self, name: str, data_source: str | None = None) -> SlayerModel | None: ... @@ -124,15 +122,14 @@ class MyCustomStorage(StorageBackend): # ``StorageBackend`` provides default implementations of ``list_models`` # and ``resolve_model_identity`` on top of ``_list_all_model_identities``. - # Call ``await self.check_datasource_id_collision(datasource.name)`` - # first to get the same case-collision protection as the built-in - # backends (optional but recommended). def save_datasource(self, datasource: DatasourceConfig) -> None: ... def get_datasource(self, name: str) -> DatasourceConfig | None: ... def list_datasources(self) -> list[str]: ... def _delete_datasource_row(self, name: str) -> bool: ... ``` +If your backend stores ids as filenames (or another case-insensitive keyspace), set the class attribute `_ids_collide_as_filenames = True` — the base save templates then reject ids differing only by case — and call `await self.check_datasource_id_collision(datasource.name)` at the top of your `save_datasource`. + Register it for URI-based resolution: ```python diff --git a/slayer/core/errors.py b/slayer/core/errors.py index 75a7c1b3..f11886c1 100644 --- a/slayer/core/errors.py +++ b/slayer/core/errors.py @@ -117,12 +117,12 @@ def __init__(self, cycle: list[tuple[str, str]]) -> None: class IdCollisionError(SlayerError, ValueError): - """Raised when saving an entity whose id differs from an existing id - only by letter case — such ids collide as filenames in the YAML - backend on case-insensitive filesystems, so every backend rejects - them. ``kind`` is ``"model"`` / ``"datasource"`` / ``"memory"``. - Multi-inherits ``ValueError`` so existing ``except ValueError`` call - sites continue to work unchanged. + """Raised by filename-backed (YAML) storage when saving an entity + whose id differs from an existing id only by letter case — such ids + collide as filenames on case-insensitive filesystems. ``kind`` is + ``"model"`` / ``"datasource"`` / ``"memory"``. Multi-inherits + ``ValueError`` so existing ``except ValueError`` call sites continue + to work unchanged. """ _LABELS = { diff --git a/slayer/storage/base.py b/slayer/storage/base.py index 12757e67..6b821838 100644 --- a/slayer/storage/base.py +++ b/slayer/storage/base.py @@ -191,6 +191,12 @@ class StorageBackend(ABC): across backends. """ + #: Backends whose ids become filenames (YAML) set this to True: saves + #: then reject ids differing only by case, which would alias to the + #: same file on case-insensitive filesystems. Wrappers copy it from + #: the inner backend. + _ids_collide_as_filenames = False + # ---- model CRUD (composite key) ---------------------------------------- async def save_model( @@ -198,9 +204,9 @@ async def save_model( ) -> None: """Persist a model. - Runs save-time validation (case-collision rejection on the model - identity, then derived-column cycle detection) and delegates to - the backend-specific :meth:`_save_model_impl`. The + Runs save-time validation (case-collision rejection for + filename-backed stores, then derived-column cycle detection) and + delegates to the backend-specific :meth:`_save_model_impl`. The ``_validate=False`` escape hatch is for trusted internal callers — currently only the migration write-back in :meth:`_migrate_and_refine_on_load` — that must persist legacy @@ -211,7 +217,8 @@ async def save_model( override this method. """ if _validate: - await self._check_model_identity_collision(model) + if self._ids_collide_as_filenames: + await self._check_model_identity_collision(model) from slayer.engine.column_dependency import validate_no_column_cycles await validate_no_column_cycles(model=model, storage=self) await self._save_model_impl(model) @@ -443,13 +450,13 @@ async def _migrate_and_refine_on_load( @abstractmethod async def save_datasource(self, datasource: DatasourceConfig) -> None: """Persist a datasource config (upsert by exact name). - Implementations should call :meth:`check_datasource_id_collision` - before writing.""" + Filename-backed implementations should call + :meth:`check_datasource_id_collision` before writing.""" async def check_datasource_id_collision(self, name: str) -> None: """Raise :class:`IdCollisionError` when ``name`` differs only by case from an existing datasource name or a saved model's - ``data_source``. Public so custom backends can call it from + ``data_source``. Public so backends can call it from ``save_datasource``.""" existing = set(await self.list_datasources()) existing.update(ds for ds, _ in await self._list_all_model_identities()) @@ -655,11 +662,6 @@ async def _next_memory_seq(self) -> str: no-leading-zero ids count toward the max walk; ``"42abc"`` and ``"001"`` are ignored. Empty corpus → ``"1"``.""" - async def _list_memory_ids(self) -> list[str]: - """Every persisted memory id; backends override with a cheap - id-only listing.""" - return [m.id for m in await self._list_memories_rows(entities=None)] - async def save_memory( self, *, @@ -674,8 +676,9 @@ async def save_memory( * ``id=None`` → allocator picks the next int-shaped id (``str``). * ``id="some-string"`` → user-supplied; rejected on bad charset or empty. Duplicate id → unconditional upsert; ``created_at`` - of the original row is preserved. An id that differs only by - case from an existing one raises :class:`IdCollisionError`. + of the original row is preserved. On filename-backed (YAML) + storage an id differing only by case from an existing one + raises :class:`IdCollisionError`. DEV-1549: ``description`` is an optional compact preview shown by ``search(compact=True)`` and ``inspect_model(compact=True)``. @@ -683,11 +686,13 @@ async def save_memory( """ if id is not None: _validate_memory_id_charset(id) - collide = _find_case_colliding_id(id, await self._list_memory_ids()) - if collide is not None: - raise IdCollisionError( - kind="memory", new_id=id, existing_id=collide, - ) + if self._ids_collide_as_filenames: + ids = [m.id for m in await self._list_memories_rows(entities=None)] + collide = _find_case_colliding_id(id, ids) + if collide is not None: + raise IdCollisionError( + kind="memory", new_id=id, existing_id=collide, + ) existing = await self._get_memory_row(id) assigned_id = id preserved_created_at = ( diff --git a/slayer/storage/join_sync.py b/slayer/storage/join_sync.py index a34f4bf4..aa6bf377 100644 --- a/slayer/storage/join_sync.py +++ b/slayer/storage/join_sync.py @@ -106,6 +106,7 @@ class JoinSyncStorage(StorageBackend): def __init__(self, inner: StorageBackend) -> None: self._inner = inner self._reconciled = False + self._ids_collide_as_filenames = inner._ids_collide_as_filenames # -- graph fingerprint ------------------------------------------------ @@ -267,9 +268,6 @@ async def _delete_memory_row(self, memory_id: str) -> bool: async def _next_memory_seq(self) -> str: return await self._inner._next_memory_seq() - async def _list_memory_ids(self) -> list[str]: - return await self._inner._list_memory_ids() - async def strip_dangling_entities_from_memories( self, *, canonical_id: str, ) -> int: diff --git a/slayer/storage/sqlite_storage.py b/slayer/storage/sqlite_storage.py index 6e3af73f..534750df 100644 --- a/slayer/storage/sqlite_storage.py +++ b/slayer/storage/sqlite_storage.py @@ -13,13 +13,11 @@ import sqlite3 from typing import Any -from slayer.core.errors import IdCollisionError from slayer.core.models import DatasourceConfig, SlayerModel from slayer.core.query import SlayerQuery from slayer.memories.models import Memory, _validate_memory_id_charset from slayer.storage.base import ( StorageBackend, - _find_case_colliding_id, _validate_path_component, _write_sample_fields, ) @@ -350,7 +348,6 @@ async def update_column_sampled( ) async def save_datasource(self, datasource: DatasourceConfig) -> None: - await self.check_datasource_id_collision(datasource.name) await asyncio.to_thread(self._save_datasource_sync, datasource) async def get_datasource(self, name: str) -> DatasourceConfig | None: @@ -437,19 +434,6 @@ def _save_memory_atomic_sync( if memory_id is None: memory_id = self._next_memory_seq_sync_from_conn(conn) else: - # Duplicated here because this override bypasses the - # base save_memory template. - ids = [ - row[0] if isinstance(row[0], str) else str(row[0]) - for row in conn.execute("SELECT id FROM memories") - ] - collide = _find_case_colliding_id(memory_id, ids) - if collide is not None: - raise IdCollisionError( - kind="memory", - new_id=memory_id, - existing_id=collide, - ) existing_row = conn.execute( "SELECT data FROM memories WHERE id = ?", (memory_id,), @@ -559,14 +543,6 @@ def _next_memory_seq_sync(self) -> str: async def _next_memory_seq(self) -> str: return await asyncio.to_thread(self._next_memory_seq_sync) - def _list_memory_ids_sync(self) -> list[str]: - with sqlite3.connect(self.db_path) as conn: - rows = conn.execute("SELECT id FROM memories").fetchall() - return [r[0] if isinstance(r[0], str) else str(r[0]) for r in rows] - - async def _list_memory_ids(self) -> list[str]: - return await asyncio.to_thread(self._list_memory_ids_sync) - def _get_memory_sync(self, memory_id: str) -> str | None: with sqlite3.connect(self.db_path) as conn: row = conn.execute( diff --git a/slayer/storage/yaml_storage.py b/slayer/storage/yaml_storage.py index beb4ff85..3e83f932 100644 --- a/slayer/storage/yaml_storage.py +++ b/slayer/storage/yaml_storage.py @@ -228,6 +228,8 @@ def migrate_memories_layout(base_dir: str) -> None: class YAMLStorage(SidecarEmbeddingsMixin, StorageBackend): + _ids_collide_as_filenames = True + def __init__(self, base_dir: str): self.base_dir = base_dir self.models_dir = os.path.join(base_dir, "models") @@ -500,9 +502,6 @@ def _memory_ids_on_disk(self) -> list[str]: if fname.endswith(".md") ] - async def _list_memory_ids(self) -> list[str]: - return self._memory_ids_on_disk() - async def _next_memory_seq(self) -> str: """DEV-1658: next int-shaped id from the ``memories/`` dir stems. Non-int stems (``help.intro``, ``kb.policy.42``, ``001``) are ignored. diff --git a/tests/test_id_collision.py b/tests/test_id_collision.py index 2dd9306f..09240516 100644 --- a/tests/test_id_collision.py +++ b/tests/test_id_collision.py @@ -1,14 +1,14 @@ """Case-collision rejection for datasource / model / memory ids. -Saving an id that differs only by case from an existing one raises -IdCollisionError on every backend; exact-id re-saves remain upserts. +Ids are filenames in the YAML backend, so saving an id that differs only +by case from an existing one raises IdCollisionError there (exact-id +re-saves remain upserts). SQLite keys are case-sensitive and stores +case-variant ids distinctly. """ from __future__ import annotations -import json import os -import sqlite3 import tempfile from collections.abc import Iterator @@ -17,9 +17,7 @@ from slayer.core.errors import IdCollisionError from slayer.core.models import DatasourceConfig, SlayerModel -from slayer.memories.models import Memory from slayer.storage.base import ( - StorageBackend, _find_case_colliding_id, _fs_equivalence_key, resolve_storage, @@ -28,13 +26,16 @@ from slayer.storage.yaml_storage import YAMLStorage -@pytest.fixture(params=["yaml", "sqlite"]) -def storage(request: pytest.FixtureRequest) -> Iterator[StorageBackend]: +@pytest.fixture +def storage() -> Iterator[YAMLStorage]: with tempfile.TemporaryDirectory() as tmpdir: - if request.param == "yaml": - yield YAMLStorage(base_dir=tmpdir) - else: - yield SQLiteStorage(db_path=os.path.join(tmpdir, "test.db")) + yield YAMLStorage(base_dir=tmpdir) + + +@pytest.fixture +def sqlite_storage() -> Iterator[SQLiteStorage]: + with tempfile.TemporaryDirectory() as tmpdir: + yield SQLiteStorage(db_path=os.path.join(tmpdir, "test.db")) def _ds(name: str, host: str = "h") -> DatasourceConfig: @@ -69,12 +70,12 @@ def test_unrelated_ids_do_not_collide(self) -> None: # --------------------------------------------------------------------------- -# Datasources +# YAML: datasources # --------------------------------------------------------------------------- class TestDatasourceCollision: - async def test_case_variant_rejected(self, storage: StorageBackend) -> None: + async def test_case_variant_rejected(self, storage: YAMLStorage) -> None: await storage.save_datasource(_ds("db", host="first")) with pytest.raises(IdCollisionError) as exc_info: await storage.save_datasource(_ds("DB", host="second")) @@ -84,14 +85,14 @@ async def test_case_variant_rejected(self, storage: StorageBackend) -> None: loaded = await storage.get_datasource("db") assert loaded.host == "first" - async def test_exact_resave_upserts(self, storage: StorageBackend) -> None: + async def test_exact_resave_upserts(self, storage: YAMLStorage) -> None: await storage.save_datasource(_ds("db", host="first")) await storage.save_datasource(_ds("db", host="second")) loaded = await storage.get_datasource("db") assert loaded.host == "second" async def test_collides_with_model_data_source( - self, storage: StorageBackend, + self, storage: YAMLStorage, ) -> None: # Models under an orphan datasource reserve its name. await storage.save_model(_model("orders", data_source="db")) @@ -101,13 +102,13 @@ async def test_collides_with_model_data_source( # --------------------------------------------------------------------------- -# Models +# YAML: models # --------------------------------------------------------------------------- class TestModelCollision: async def test_case_variant_name_rejected( - self, storage: StorageBackend, + self, storage: YAMLStorage, ) -> None: await storage.save_model(_model("orders", table="first")) with pytest.raises(IdCollisionError) as exc_info: @@ -119,14 +120,14 @@ async def test_case_variant_name_rejected( loaded = await storage.get_model("orders", data_source="db") assert loaded.sql_table == "first" - async def test_exact_resave_upserts(self, storage: StorageBackend) -> None: + async def test_exact_resave_upserts(self, storage: YAMLStorage) -> None: await storage.save_model(_model("orders", table="first")) await storage.save_model(_model("orders", table="second")) loaded = await storage.get_model("orders", data_source="db") assert loaded.sql_table == "second" async def test_same_name_in_other_datasource_ok( - self, storage: StorageBackend, + self, storage: YAMLStorage, ) -> None: await storage.save_model(_model("orders", data_source="db_a")) await storage.save_model(_model("Orders", data_source="db_b")) @@ -134,7 +135,7 @@ async def test_same_name_in_other_datasource_ok( assert await storage.get_model("Orders", data_source="db_b") is not None async def test_case_variant_data_source_rejected( - self, storage: StorageBackend, + self, storage: YAMLStorage, ) -> None: await storage.save_datasource(_ds("db")) with pytest.raises(IdCollisionError) as exc_info: @@ -142,27 +143,25 @@ async def test_case_variant_data_source_rejected( assert exc_info.value.kind == "datasource" async def test_data_source_vs_other_models_rejected( - self, storage: StorageBackend, + self, storage: YAMLStorage, ) -> None: await storage.save_model(_model("orders", data_source="db")) with pytest.raises(IdCollisionError): await storage.save_model(_model("customers", data_source="DB")) - async def test_validate_false_bypasses( - self, storage: StorageBackend, - ) -> None: + async def test_validate_false_bypasses(self, storage: YAMLStorage) -> None: # The migration write-back path must stay able to persist legacy data. await storage.save_model(_model("orders")) await storage.save_model(_model("Orders"), _validate=False) # --------------------------------------------------------------------------- -# Memories (rejection also covered in test_memory_string_ids.py) +# YAML: memories (rejection also covered in test_memory_string_ids.py) # --------------------------------------------------------------------------- class TestMemoryCollision: - async def test_exact_upsert_allowed(self, storage: StorageBackend) -> None: + async def test_exact_upsert_allowed(self, storage: YAMLStorage) -> None: await storage.save_memory( id="kb.x", learning="one", entities=["mydb.orders"], ) @@ -171,7 +170,7 @@ async def test_exact_upsert_allowed(self, storage: StorageBackend) -> None: ) assert (await storage.get_memory("kb.x")).learning == "two" - async def test_error_attrs(self, storage: StorageBackend) -> None: + async def test_error_attrs(self, storage: YAMLStorage) -> None: await storage.save_memory( id="Kb.X", learning="one", entities=["mydb.orders"], ) @@ -185,49 +184,44 @@ async def test_error_attrs(self, storage: StorageBackend) -> None: # --------------------------------------------------------------------------- -# Legacy stores already holding a colliding pair: either save must raise +# SQLite: case-variant ids are distinct identities, no rejection # --------------------------------------------------------------------------- -class TestLegacyCollidingPair: - async def test_sqlite_memory_pair_blocks_both(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - db_path = os.path.join(tmpdir, "test.db") - storage = SQLiteStorage(db_path=db_path) - with sqlite3.connect(db_path) as conn: - for mid, learning in (("X", "upper"), ("x", "lower")): - m = Memory(id=mid, learning=learning, entities=[]) - conn.execute( - "INSERT INTO memories (id, data) VALUES (?, ?)", - (mid, json.dumps(m.model_dump(mode="json"))), - ) - for mid in ("X", "x"): - with pytest.raises(IdCollisionError): - await storage.save_memory( - id=mid, learning="update", entities=[], - ) - assert (await storage.get_memory("X")).learning == "upper" - assert (await storage.get_memory("x")).learning == "lower" +class TestSqliteAllowsCaseVariants: + async def test_datasources(self, sqlite_storage: SQLiteStorage) -> None: + await sqlite_storage.save_datasource(_ds("db", host="lower")) + await sqlite_storage.save_datasource(_ds("DB", host="upper")) + assert (await sqlite_storage.get_datasource("db")).host == "lower" + assert (await sqlite_storage.get_datasource("DB")).host == "upper" + + async def test_models(self, sqlite_storage: SQLiteStorage) -> None: + await sqlite_storage.save_model(_model("orders", table="lower")) + await sqlite_storage.save_model(_model("Orders", table="upper")) + low = await sqlite_storage.get_model("orders", data_source="db") + up = await sqlite_storage.get_model("Orders", data_source="db") + assert low.sql_table == "lower" + assert up.sql_table == "upper" + + async def test_memories(self, sqlite_storage: SQLiteStorage) -> None: + await sqlite_storage.save_memory( + id="X", learning="upper", entities=["mydb.orders"], + ) + await sqlite_storage.save_memory( + id="x", learning="lower", entities=["mydb.orders"], + ) + assert (await sqlite_storage.get_memory("X")).learning == "upper" + assert (await sqlite_storage.get_memory("x")).learning == "lower" - async def test_sqlite_model_pair_blocks_both(self) -> None: + async def test_wrapped_sqlite_allows(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: - db_path = os.path.join(tmpdir, "test.db") - storage = SQLiteStorage(db_path=db_path) - with sqlite3.connect(db_path) as conn: - for name in ("orders", "Orders"): - data = _model(name).model_dump(mode="json", exclude_none=True) - conn.execute( - "INSERT INTO models (data_source, name, data) " - "VALUES (?, ?, ?)", - ("db", name, json.dumps(data)), - ) - for name in ("orders", "Orders"): - with pytest.raises(IdCollisionError): - await storage.save_model(_model(name)) + storage = resolve_storage(os.path.join(tmpdir, "test.db")) + await storage.save_model(_model("orders")) + await storage.save_model(_model("Orders")) # --------------------------------------------------------------------------- -# The production wrapper (resolve_storage → JoinSyncStorage) +# The production wrapper (resolve_storage → JoinSyncStorage) over YAML # --------------------------------------------------------------------------- diff --git a/tests/test_memory_string_ids.py b/tests/test_memory_string_ids.py index 056295a1..c341bfc7 100644 --- a/tests/test_memory_string_ids.py +++ b/tests/test_memory_string_ids.py @@ -180,20 +180,22 @@ async def test_save_with_user_id(self, storage: StorageBackend) -> None: loaded = await storage.get_memory("kb.policy.42") assert loaded.learning == "x" - async def test_case_colliding_ids_rejected( - self, storage: StorageBackend, - ) -> None: + async def test_case_variant_ids(self, storage: StorageBackend) -> None: + # YAML rejects (ids are filenames); SQLite stores both distinctly. await storage.save_memory( id="X", learning="upper", entities=["mydb.orders"], ) - with pytest.raises(IdCollisionError) as exc_info: + if isinstance(storage, YAMLStorage): + with pytest.raises(IdCollisionError): + await storage.save_memory( + id="x", learning="lower", entities=["mydb.orders"], + ) + else: await storage.save_memory( id="x", learning="lower", entities=["mydb.orders"], ) - assert exc_info.value.new_id == "x" - assert exc_info.value.existing_id == "X" - upper = await storage.get_memory("X") - assert upper.learning == "upper" + assert (await storage.get_memory("x")).learning == "lower" + assert (await storage.get_memory("X")).learning == "upper" async def test_zero_user_id_distinct_from_auto( self, storage: StorageBackend, From 8ad935fa32635fc13124375c35bb029cbc28e008 Mon Sep 17 00:00:00 2001 From: whimo Date: Wed, 22 Jul 2026 19:08:14 +0400 Subject: [PATCH 5/5] Address review: on-disk pair pre-check, async docs example, kwarg calls - migrate_memories_layout now also rejects a case-colliding pair already present among memories/ stems (possible on case-sensitive filesystems), instead of letting listdir order decide which spelling wins the index - docs custom-backend example methods are async, matching the contract - multi-parameter helper calls use keyword arguments per convention Claude-Session: https://claude.ai/code/session_011sbQKNXcUtGAsCXtmJXHvR --- docs/configuration/storage.md | 16 ++++++++-------- slayer/storage/base.py | 12 ++++++++---- slayer/storage/v4_migration.py | 3 ++- slayer/storage/yaml_storage.py | 35 ++++++++++++++++++++++++---------- tests/test_id_collision.py | 26 +++++++++++++++++++++++++ 5 files changed, 69 insertions(+), 23 deletions(-) diff --git a/docs/configuration/storage.md b/docs/configuration/storage.md index 1b478557..b8e5eb58 100644 --- a/docs/configuration/storage.md +++ b/docs/configuration/storage.md @@ -114,18 +114,18 @@ class MyCustomStorage(StorageBackend): # Models are keyed by (data_source, name). ``save_model`` is a template # method on the base class (it runs shared validation); backends # implement only ``_save_model_impl``. - def _save_model_impl(self, model: SlayerModel) -> None: ... - def _list_all_model_identities(self) -> list[tuple[str, str]]: ... - def get_model(self, name: str, data_source: str | None = None) -> SlayerModel | None: ... - def _delete_model_row(self, *, data_source: str, name: str) -> bool: ... + async def _save_model_impl(self, model: SlayerModel) -> None: ... + async def _list_all_model_identities(self) -> list[tuple[str, str]]: ... + async def get_model(self, name: str, data_source: str | None = None) -> SlayerModel | None: ... + async def _delete_model_row(self, *, data_source: str, name: str) -> bool: ... # ``StorageBackend`` provides default implementations of ``list_models`` # and ``resolve_model_identity`` on top of ``_list_all_model_identities``. - def save_datasource(self, datasource: DatasourceConfig) -> None: ... - def get_datasource(self, name: str) -> DatasourceConfig | None: ... - def list_datasources(self) -> list[str]: ... - def _delete_datasource_row(self, name: str) -> bool: ... + async def save_datasource(self, datasource: DatasourceConfig) -> None: ... + async def get_datasource(self, name: str) -> DatasourceConfig | None: ... + async def list_datasources(self) -> list[str]: ... + async def _delete_datasource_row(self, name: str) -> bool: ... ``` If your backend stores ids as filenames (or another case-insensitive keyspace), set the class attribute `_ids_collide_as_filenames = True` — the base save templates then reject ids differing only by case — and call `await self.check_datasource_id_collision(datasource.name)` at the top of your `save_datasource`. diff --git a/slayer/storage/base.py b/slayer/storage/base.py index 6b821838..1b2d4ae1 100644 --- a/slayer/storage/base.py +++ b/slayer/storage/base.py @@ -230,13 +230,17 @@ async def _check_model_identity_collision(self, model: SlayerModel) -> None: identities = await self._list_all_model_identities() known_ds = {ds for ds, _ in identities} known_ds.update(await self.list_datasources()) - collide = _find_case_colliding_id(model.data_source, known_ds) + collide = _find_case_colliding_id( + candidate=model.data_source, existing=known_ds, + ) if collide is not None: raise IdCollisionError( kind="datasource", new_id=model.data_source, existing_id=collide, ) names_in_ds = [n for ds, n in identities if ds == model.data_source] - collide = _find_case_colliding_id(model.name, names_in_ds) + collide = _find_case_colliding_id( + candidate=model.name, existing=names_in_ds, + ) if collide is not None: raise IdCollisionError( kind="model", @@ -460,7 +464,7 @@ async def check_datasource_id_collision(self, name: str) -> None: ``save_datasource``.""" existing = set(await self.list_datasources()) existing.update(ds for ds, _ in await self._list_all_model_identities()) - collide = _find_case_colliding_id(name, existing) + collide = _find_case_colliding_id(candidate=name, existing=existing) if collide is not None: raise IdCollisionError( kind="datasource", new_id=name, existing_id=collide, @@ -688,7 +692,7 @@ async def save_memory( _validate_memory_id_charset(id) if self._ids_collide_as_filenames: ids = [m.id for m in await self._list_memories_rows(entities=None)] - collide = _find_case_colliding_id(id, ids) + collide = _find_case_colliding_id(candidate=id, existing=ids) if collide is not None: raise IdCollisionError( kind="memory", new_id=id, existing_id=collide, diff --git a/slayer/storage/v4_migration.py b/slayer/storage/v4_migration.py index 2bdd061c..11309da6 100644 --- a/slayer/storage/v4_migration.py +++ b/slayer/storage/v4_migration.py @@ -99,6 +99,7 @@ def _yaml_list_datasource_names(datasources_dir: str) -> list[str]: def _check_layout_case_collisions( + *, models_dir: str, planned: list[tuple[str, dict, str, str]], ) -> None: @@ -172,7 +173,7 @@ def migrate_yaml_layout(base_dir: str) -> None: data["data_source"] = ds planned.append((path, data, ds, filename)) - _check_layout_case_collisions(models_dir, planned) + _check_layout_case_collisions(models_dir=models_dir, planned=planned) # Pass 2: perform the moves. for path, data, ds, filename in planned: diff --git a/slayer/storage/yaml_storage.py b/slayer/storage/yaml_storage.py index 3e83f932..208e9394 100644 --- a/slayer/storage/yaml_storage.py +++ b/slayer/storage/yaml_storage.py @@ -201,6 +201,12 @@ def migrate_memories_layout(base_dir: str) -> None: for fname in os.listdir(mem_dir): if fname.endswith(".md"): stem = fname[: -len(".md")] + prior = id_by_key.get(stem.casefold()) + if prior is not None and prior != stem: + raise ValueError( + f"{mem_dir}: memory ids {prior!r} and {stem!r} differ " + f"only by case. Rename one, then reopen." + ) id_by_key[stem.casefold()] = stem for r in normalized: rid = str(r["id"]) @@ -296,12 +302,13 @@ async def graph_fingerprint(self) -> str: def _model_path(self, data_source: str, name: str) -> str: return os.path.join(self.models_dir, data_source, f"{name}.yaml") - def _model_entry_exists(self, data_source: str, name: str) -> bool: + def _model_entry_exists(self, *, data_source: str, name: str) -> bool: """Exact-case existence check for both path components of a model.""" return _exact_entry_exists( - self.models_dir, data_source, + dir_path=self.models_dir, entry_name=data_source, ) and _exact_entry_exists( - os.path.join(self.models_dir, data_source), f"{name}.yaml", + dir_path=os.path.join(self.models_dir, data_source), + entry_name=f"{name}.yaml", ) # ---- model CRUD -------------------------------------------------------- @@ -337,7 +344,7 @@ async def get_model( return None data_source, name = target path = self._model_path(data_source, name) # NOSONAR(S6549) — name/data_source were sanitized by _resolve_target_or_none above (rejects '..', path separators, NULs); SlayerModel Pydantic validators sanitize the save path - if not self._model_entry_exists(data_source, name): + if not self._model_entry_exists(data_source=data_source, name=name): return None try: with open(path) as f: @@ -357,7 +364,7 @@ async def _delete_model_row( self, *, data_source: str, name: str, ) -> bool: # Exact match — os.remove would otherwise hit a case-variant sibling. - if not self._model_entry_exists(data_source, name): + if not self._model_entry_exists(data_source=data_source, name=name): return False os.remove(self._model_path(data_source, name)) return True @@ -373,7 +380,7 @@ async def update_column_sampled( distinct_count: int | None, ) -> None: path = self._model_path(data_source, model_name) - if not self._model_entry_exists(data_source, model_name): + if not self._model_entry_exists(data_source=data_source, name=model_name): raise ValueError( f"update_column_sampled: model {model_name!r} in datasource " f"{data_source!r} not found." @@ -411,7 +418,9 @@ async def get_datasource(self, name: str) -> DatasourceConfig | None: # DEV-1405: sanitize before composing the filesystem path. _validate_path_component(name, kind="datasource name") path = os.path.join(self.datasources_dir, f"{name}.yaml") - if not _exact_entry_exists(self.datasources_dir, f"{name}.yaml"): + if not _exact_entry_exists( + dir_path=self.datasources_dir, entry_name=f"{name}.yaml", + ): return None try: with open(path) as f: @@ -435,7 +444,9 @@ async def list_datasources(self) -> list[str]: return result async def _delete_datasource_row(self, name: str) -> bool: - if not _exact_entry_exists(self.datasources_dir, f"{name}.yaml"): + if not _exact_entry_exists( + dir_path=self.datasources_dir, entry_name=f"{name}.yaml", + ): return False os.remove(os.path.join(self.datasources_dir, f"{name}.yaml")) return True @@ -570,7 +581,9 @@ async def _get_memory_row(self, memory_id: str) -> Memory | None: path = self._memory_md_path(memory_id) # Filename IS the identity; exact check keeps a case-variant # lookup from opening the wrong file. - if not _exact_entry_exists(self._memories_dir, f"{memory_id}.md"): + if not _exact_entry_exists( + dir_path=self._memories_dir, entry_name=f"{memory_id}.md", + ): return None try: with open(path, encoding="utf-8") as f: # NOSONAR(S7493) — sync I/O in async by design @@ -601,7 +614,9 @@ async def _list_memories_rows( async def _delete_memory_row(self, memory_id: str) -> bool: with self._memories_file_lock(): path = self._memory_md_path(memory_id) - if not _exact_entry_exists(self._memories_dir, f"{memory_id}.md"): + if not _exact_entry_exists( + dir_path=self._memories_dir, entry_name=f"{memory_id}.md", + ): return False os.remove(path) return True diff --git a/tests/test_id_collision.py b/tests/test_id_collision.py index 09240516..b647316a 100644 --- a/tests/test_id_collision.py +++ b/tests/test_id_collision.py @@ -272,6 +272,32 @@ def test_legacy_memories_yaml_case_pair_refused(self) -> None: ) assert md_files == [] + def test_existing_on_disk_memory_pair_refused( + self, monkeypatch: pytest.MonkeyPatch, + ) -> None: + # X.md and x.md can coexist only on a case-sensitive FS; simulate + # via listdir so the check is pinned on any dev machine. + with tempfile.TemporaryDirectory() as tmpdir: + legacy = os.path.join(tmpdir, "memories.yaml") + with open(legacy, "w") as f: + yaml.safe_dump( + [{"id": "other", "learning": "l", "entities": []}], f, + ) + mem_dir = os.path.join(tmpdir, "memories") + real_listdir = os.listdir + + def fake_listdir(path): + if os.path.abspath(path) == os.path.abspath(mem_dir): + return ["X.md", "x.md"] + return real_listdir(path) + + monkeypatch.setattr( + "slayer.storage.yaml_storage.os.listdir", fake_listdir, + ) + with pytest.raises(ValueError, match="differ only by case"): + YAMLStorage(base_dir=tmpdir) + assert os.path.exists(legacy) + def test_flat_models_case_variant_datasources_refused(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: models_dir = os.path.join(tmpdir, "models")