diff --git a/CLAUDE.md b/CLAUDE.md index 75511500..7d5a17b9 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, 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 934d59aa..09cff708 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. +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. ### Search side effects diff --git a/docs/concepts/memories.md b/docs/concepts/memories.md index 23a6edd1..437e8070 100644 --- a/docs/concepts/memories.md +++ b/docs/concepts/memories.md @@ -77,7 +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. +`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 b6e662d8..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 | +| `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 3d8e5739..b8e5eb58 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 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. **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. @@ -109,21 +111,25 @@ 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: ... - 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: ... + # 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``. + 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(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`. + Register it for URI-based resolution: ```python 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..f11886c1 100644 --- a/slayer/core/errors.py +++ b/slayer/core/errors.py @@ -116,6 +116,41 @@ def __init__(self, cycle: list[tuple[str, str]]) -> None: super().__init__(f"Circular column reference detected: {chain}") +class IdCollisionError(SlayerError, ValueError): + """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 = { + "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..1fa09ee6 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,33 @@ 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 — report and continue. + 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..1b2d4ae1 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,26 @@ def _entity_matches_cascade( return entry.startswith(f"{canonical_id}.") +def _fs_equivalence_key(value: str) -> str: + """Key under which two ids collide on a case-insensitive filesystem.""" + 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 + (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: + 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. @@ -167,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( @@ -174,22 +204,51 @@ 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 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 + 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: + 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) + async def _check_model_identity_collision(self, model: SlayerModel) -> None: + """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()) + 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( + candidate=model.name, existing=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. @@ -393,7 +452,23 @@ async def _migrate_and_refine_on_load( # ---- datasource CRUD --------------------------------------------------- @abstractmethod - async def save_datasource(self, datasource: DatasourceConfig) -> None: ... + async def save_datasource(self, datasource: DatasourceConfig) -> None: + """Persist a datasource config (upsert by exact name). + 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 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(candidate=name, existing=existing) + if collide is not None: + raise IdCollisionError( + kind="datasource", new_id=name, existing_id=collide, + ) @abstractmethod async def get_datasource(self, name: str) -> DatasourceConfig | None: ... @@ -605,7 +680,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. + 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)``. @@ -613,6 +690,13 @@ async def save_memory( """ if id is not None: _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(candidate=id, existing=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..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 ------------------------------------------------ diff --git a/slayer/storage/v4_migration.py b/slayer/storage/v4_migration.py index 7970a38e..11309da6 100644 --- a/slayer/storage/v4_migration.py +++ b/slayer/storage/v4_migration.py @@ -98,6 +98,45 @@ 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 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): + 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 +160,9 @@ def migrate_yaml_layout(base_dir: str) -> None: available = _yaml_list_datasource_names(datasources_dir) + # 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) with open(path) as f: @@ -129,6 +171,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=models_dir, planned=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..208e9394 100644 --- a/slayer/storage/yaml_storage.py +++ b/slayer/storage/yaml_storage.py @@ -105,6 +105,16 @@ 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`` — 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): + return False + + def _normalize_legacy_memory_rows( rows: list[dict[str, Any]], ) -> list[dict[str, Any]]: @@ -184,6 +194,29 @@ def migrate_memories_layout(base_dir: str) -> None: ) normalized = _normalize_legacy_memory_rows(rows) mem_dir = os.path.join(base_dir, "memories") + # 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): + 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"]) + 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) @@ -201,6 +234,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") @@ -267,6 +302,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.""" + return _exact_entry_exists( + dir_path=self.models_dir, entry_name=data_source, + ) and _exact_entry_exists( + dir_path=os.path.join(self.models_dir, data_source), + entry_name=f"{name}.yaml", + ) + # ---- model CRUD -------------------------------------------------------- async def _save_model_impl(self, model: SlayerModel) -> None: @@ -299,8 +343,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=data_source, name=name): return None try: with open(path) as f: @@ -319,11 +363,11 @@ 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 — os.remove would otherwise hit a case-variant sibling. + if not self._model_entry_exists(data_source=data_source, name=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=data_source, name=model_name): raise ValueError( f"update_column_sampled: model {model_name!r} in datasource " f"{data_source!r} not found." @@ -364,6 +408,7 @@ async def update_column_sampled( # ---- datasource CRUD --------------------------------------------------- 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: @@ -373,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 os.path.exists(path): + if not _exact_entry_exists( + dir_path=self.datasources_dir, entry_name=f"{name}.yaml", + ): return None try: with open(path) as f: @@ -397,11 +444,12 @@ 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( + 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 # ---- datasource priority ----------------------------------------------- @@ -531,6 +579,12 @@ 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) + # Filename IS the identity; exact check keeps a case-variant + # lookup from opening the wrong file. + 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 return _md_to_memory(memory_id, f.read()) @@ -560,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 os.path.exists(path): + 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_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..b647316a --- /dev/null +++ b/tests/test_id_collision.py @@ -0,0 +1,330 @@ +"""Case-collision rejection for datasource / model / memory ids. + +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 os +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.storage.base import ( + _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 +def storage() -> Iterator[YAMLStorage]: + with tempfile.TemporaryDirectory() as tmpdir: + 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: + 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: + 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 + + +# --------------------------------------------------------------------------- +# YAML: datasources +# --------------------------------------------------------------------------- + + +class TestDatasourceCollision: + 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")) + 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: 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: YAMLStorage, + ) -> None: + # 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")) + await storage.save_datasource(_ds("db")) + + +# --------------------------------------------------------------------------- +# YAML: models +# --------------------------------------------------------------------------- + + +class TestModelCollision: + async def test_case_variant_name_rejected( + self, storage: YAMLStorage, + ) -> 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: 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: YAMLStorage, + ) -> None: + 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: YAMLStorage, + ) -> 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: 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: 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) + + +# --------------------------------------------------------------------------- +# YAML: memories (rejection also covered in test_memory_string_ids.py) +# --------------------------------------------------------------------------- + + +class TestMemoryCollision: + async def test_exact_upsert_allowed(self, storage: YAMLStorage) -> 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: YAMLStorage) -> 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" + + +# --------------------------------------------------------------------------- +# SQLite: case-variant ids are distinct identities, no rejection +# --------------------------------------------------------------------------- + + +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_wrapped_sqlite_allows(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + 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) over YAML +# --------------------------------------------------------------------------- + + +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) + 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_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") + 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) + 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..c341bfc7 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,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_sensitive_ids( - 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"], ) - await storage.save_memory( - id="x", learning="lower", entities=["mydb.orders"], - ) - upper = await storage.get_memory("X") - lower = await storage.get_memory("x") - assert upper.learning == "upper" - assert lower.learning == "lower" + 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 (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, diff --git a/tests/test_yaml_exact_entry.py b/tests/test_yaml_exact_entry.py new file mode 100644 index 00000000..efc2190c --- /dev/null +++ b/tests/test_yaml_exact_entry.py @@ -0,0 +1,122 @@ +"""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 + +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"