diff --git a/docs/concepts/memories.md b/docs/concepts/memories.md index 23a6edd1..355ec5a9 100644 --- a/docs/concepts/memories.md +++ b/docs/concepts/memories.md @@ -173,13 +173,16 @@ For retrieval, see [`search`](search.md) (MCP `search`, REST `POST ## Storage layout -YAML stores one Markdown file per memory at `memories/.md` — YAML -frontmatter for the structured fields (`description`, `entities`, -`query`, `created_at`, `version`) and the Markdown body as the -`learning`. The id is the filename (not repeated in the frontmatter). -A legacy flat `memories.yaml` is migrated into per-file `.md` on first -open (and then deleted). SQLite uses a `memories` table plus a -`memory_entities` index table for the entity-overlap filter. +YAML stores one Markdown file per memory. Portable lowercase ASCII ids +use `memories/.md`; other ids use +`memories/.encoded/.md`, which prevents filesystem case folding +or Unicode normalization from merging distinct ids. YAML frontmatter holds +the structured fields (`description`, `entities`, `query`, `created_at`, +`version`) and the Markdown body is the `learning`. The id is derived from the +reversible filename (not repeated in the frontmatter). A legacy flat +`memories.yaml` is migrated into per-file `.md` on first open (and then +deleted). SQLite uses a `memories` table plus a `memory_entities` index table +for the entity-overlap filter. IDs are non-empty strings (DEV-1428). The auto-allocator walks `max(int-shaped id) + 1` over the existing corpus where "int-shaped" diff --git a/docs/concepts/models.md b/docs/concepts/models.md index b6e662d8..f5bbb9c6 100644 --- a/docs/concepts/models.md +++ b/docs/concepts/models.md @@ -1,6 +1,6 @@ # Models -A model is SLayer's view of a database table or an underlying SQL query. It declares the columns, named metric formulas, joins, and always-applied filters that queries can build on. Models are defined as YAML (one file per model under `models//`) or created via API/MCP — the two paths produce the same persisted object. +A model is SLayer's view of a database table or an underlying SQL query. It declares the columns, named metric formulas, joins, and always-applied filters that queries can build on. Models are defined as YAML (one file per model under `models//`) or created via API/MCP — the two paths produce the same persisted object. Non-portable ids use reversible UTF-8 hex path components under `models/.encoded/` so filesystem normalization cannot merge distinct model identities. A tiny example to anchor what follows: diff --git a/docs/configuration/datasources.md b/docs/configuration/datasources.md index 33f424da..8e8312fd 100644 --- a/docs/configuration/datasources.md +++ b/docs/configuration/datasources.md @@ -1,6 +1,10 @@ # Datasources -Datasources configure database connections. They are stored as individual YAML files in the `datasources/` directory. +Datasources configure database connections. They are stored as individual YAML +files in the `datasources/` directory. Human-authored, portable lowercase +filenames keep the layout shown below; ids that are not portable across +filesystems use a reversible UTF-8 hex filename under +`datasources/.encoded/`. ## YAML Format diff --git a/docs/configuration/storage.md b/docs/configuration/storage.md index 3d8e5739..744aba7f 100644 --- a/docs/configuration/storage.md +++ b/docs/configuration/storage.md @@ -64,6 +64,17 @@ 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. +Lowercase ASCII ids containing only letters, digits, `.`, `_`, or `-` keep the +readable paths shown above, except Windows-reserved device names. Other +ids—and a portable id that would collide with an existing legacy filename—are +stored under a reserved `.encoded/` directory using reversible UTF-8 hex. +Models use +`models/.encoded//.yaml`; datasource configs and +memories use `datasources/.encoded/.yaml` and +`memories/.encoded/.md`. This keeps distinct ids such as `X` and `x` +distinct on case-insensitive filesystems. Existing human-authored paths remain +readable, but legacy lookup requires an exact spelling match. + **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. diff --git a/slayer/storage/yaml_storage.py b/slayer/storage/yaml_storage.py index ef6619ab..e1b7ce88 100644 --- a/slayer/storage/yaml_storage.py +++ b/slayer/storage/yaml_storage.py @@ -9,6 +9,11 @@ file into the new subdirectory. See ``slayer/storage/v4_migration.py`` for the contract details. +Portable lowercase ASCII ids retain that readable layout. Other ids use a +reversible UTF-8 hex filename below a reserved ``.encoded`` directory, so +case folding and Unicode normalization cannot alias distinct logical ids. +Legacy paths remain readable only when their spelling matches exactly. + DEV-1405: embedding rows now live in a SQLite sidecar at ``/embeddings.db`` (via :class:`SidecarEmbeddingStore`) instead of a single ``embeddings.yaml`` whose whole-file-rewrite-on-save bottlenecked @@ -22,6 +27,7 @@ """ import contextlib +import filecmp import os from typing import Any from collections.abc import Iterator @@ -50,10 +56,176 @@ _LEGACY_RENAMES = ("embeddings.yaml", "counters.yaml") _YAML_EXTS = (".yaml", ".yml") # NOSONAR(S1192) — full filenames in _LEGACY_RENAMES are semantically distinct from this extension tuple +_ENCODED_IDS_DIR = ".encoded" +_PORTABLE_STORAGE_ID_CHARS = frozenset( + "abcdefghijklmnopqrstuvwxyz0123456789._-" +) +_HEX_CHARS = frozenset("0123456789abcdef") +_WINDOWS_RESERVED_NAMES = frozenset( + {"con", "prn", "aux", "nul"} + | {f"com{i}" for i in range(1, 10)} + | {f"lpt{i}" for i in range(1, 10)} +) _MD_FENCE = "---\n" +# ---- filesystem-safe logical ids ------------------------------------------ + + +def _encode_storage_id(value: str) -> str: + """Return a reversible, filesystem-independent filename component.""" + return value.encode("utf-8").hex() + + +def _decode_storage_id(value: str) -> str | None: + """Decode :func:`_encode_storage_id`; ignore unrelated/corrupt entries.""" + if not value or any(ch not in _HEX_CHARS for ch in value): + return None + try: + return bytes.fromhex(value).decode("utf-8") + except (UnicodeDecodeError, ValueError): + return None + + +def _is_portable_storage_id(value: str) -> bool: + """Whether ``value`` is already stable on supported filesystems.""" + return ( + bool(value) + and all(ch in _PORTABLE_STORAGE_ID_CHARS for ch in value) + and value.split(".", 1)[0] not in _WINDOWS_RESERVED_NAMES + ) + + +def _exact_child_path(parent: str, name: str) -> str | None: + """Return a child only when its spelling exactly matches ``name``. + + ``exists(parent/name)`` is insufficient on case-insensitive filesystems: + asking for ``x.yaml`` succeeds when only ``X.yaml`` exists. + """ + try: + if name not in os.listdir(parent): + return None + except FileNotFoundError: + return None + return os.path.join(parent, name) + + +def _encoded_file_path(parent: str, value: str, suffix: str) -> str: + return os.path.join( + parent, _ENCODED_IDS_DIR, f"{_encode_storage_id(value)}{suffix}", + ) + + +def _storage_file_path(parent: str, value: str, suffix: str) -> str: + """Choose a portable raw path, or encoded storage when raw would alias.""" + encoded = _encoded_file_path(parent, value, suffix) + if os.path.isfile(encoded) or not _is_portable_storage_id(value): + return encoded + raw = os.path.join(parent, f"{value}{suffix}") + exact = _exact_child_path(parent, f"{value}{suffix}") + if exact is not None: + return exact + return encoded if os.path.exists(raw) else raw + + +def _move_identity_files(moves: list[tuple[str, str]]) -> None: + """Preflight every move, then atomically relocate the legacy files.""" + sources_by_target: dict[str, str] = {} + for source, target in moves: + prior = sources_by_target.setdefault(target, source) + if prior != source: + raise ValueError( + f"Cannot migrate YAML storage identities: {prior!r} and " + f"{source!r} map to the same target {target!r}." + ) + if os.path.exists(target) and ( + not os.path.isfile(target) + or not filecmp.cmp(source, target, shallow=False) + ): + raise ValueError( + f"Cannot migrate YAML storage identity {source!r}: target " + f"{target!r} already exists with different content." + ) + + for source, target in moves: + os.makedirs(os.path.dirname(target), exist_ok=True) + if os.path.exists(target): + with contextlib.suppress(FileNotFoundError): + os.remove(source) + continue + try: + os.replace(source, target) + except FileNotFoundError: + # A concurrent opener may have completed the same atomic move. + if not os.path.isfile(target): + raise + + +def migrate_identity_paths(base_dir: str) -> None: + """Move non-portable legacy filenames into the encoded namespace.""" + moves: list[tuple[str, str]] = [] + + datasources_dir = os.path.join(base_dir, "datasources") + for filename in os.listdir(datasources_dir): + if not filename.endswith(".yaml"): + continue + name = filename[: -len(".yaml")] + target = _encoded_file_path(datasources_dir, name, ".yaml") + if _is_portable_storage_id(name) and not os.path.exists(target): + continue + moves.append(( + os.path.join(datasources_dir, filename), + target, + )) + + models_dir = os.path.join(base_dir, "models") + for data_source in os.listdir(models_dir): + if data_source == _ENCODED_IDS_DIR: + continue + ds_dir = os.path.join(models_dir, data_source) + if not os.path.isdir(ds_dir): + continue + for filename in os.listdir(ds_dir): + if not filename.endswith(".yaml"): + continue + name = filename[: -len(".yaml")] + target = os.path.join( + models_dir, + _ENCODED_IDS_DIR, + _encode_storage_id(data_source), + f"{_encode_storage_id(name)}.yaml", + ) + if ( + _is_portable_storage_id(data_source) + and _is_portable_storage_id(name) + and not os.path.exists(target) + ): + continue + moves.append(( + os.path.join(ds_dir, filename), + target, + )) + + memories_dir = os.path.join(base_dir, "memories") + for filename in os.listdir(memories_dir): + if not filename.endswith(".md"): + continue + memory_id = filename[: -len(".md")] + target = _encoded_file_path(memories_dir, memory_id, ".md") + if ( + _is_portable_storage_id(memory_id) + and not os.path.exists(target) + ): + continue + moves.append(( + os.path.join(memories_dir, filename), + target, + )) + + _move_identity_files(moves) + + # ---- memory <-> .md (DEV-1658) -------------------------------------------- @@ -141,7 +313,7 @@ def _normalize_legacy_memory_rows( def migrate_memories_layout(base_dir: str) -> None: """DEV-1658: one-time migration of a legacy flat ``memories.yaml`` into - per-id ``memories/.md`` files, then delete the legacy file. + per-id Markdown files, then delete the legacy file. Fails loud (raises, legacy file preserved) on invalid YAML, a non-list root, or a non-dict row — a corrupt file must never be treated as empty @@ -184,12 +356,15 @@ def migrate_memories_layout(base_dir: str) -> None: ) normalized = _normalize_legacy_memory_rows(rows) mem_dir = os.path.join(base_dir, "memories") - os.makedirs(mem_dir, exist_ok=True) for r in normalized: mem = Memory.model_validate(r) - _atomic_write_text( - os.path.join(mem_dir, f"{mem.id}.md"), _memory_to_md(mem), - ) + target = _storage_file_path(mem_dir, mem.id, ".md") + os.makedirs(os.path.dirname(target), exist_ok=True) + _atomic_write_text(target, _memory_to_md(mem)) + legacy_path = _exact_child_path(mem_dir, f"{mem.id}.md") + if legacy_path is not None and legacy_path != target: + with contextlib.suppress(FileNotFoundError): + os.remove(legacy_path) # Guard the removal against a concurrent migrator (two workers opening the # same fresh base_dir both run this once): the .md writes are atomic and # idempotent, so only the double os.remove would crash. FileNotFoundError @@ -225,6 +400,9 @@ def __init__(self, base_dir: str): # per-id ``.md`` files. Fails loud on a corrupt/non-list legacy file # (never deletes it); a crash mid-migration re-runs cleanly. migrate_memories_layout(base_dir) + # Atomically move case/normalization-sensitive legacy paths into the + # reversible encoded namespace. All collisions are checked first. + migrate_identity_paths(base_dir) # Idempotent — rename pre-DEV-1405 sidecar files out of the way. # If a ``.legacy`` companion already exists (user upgraded twice or # manually restored), leave both files in place so we never clobber @@ -264,31 +442,101 @@ async def graph_fingerprint(self) -> str: # ---- internal helpers -------------------------------------------------- + def _encoded_model_path(self, data_source: str, name: str) -> str: + return os.path.join( + self.models_dir, + _ENCODED_IDS_DIR, + _encode_storage_id(data_source), + f"{_encode_storage_id(name)}.yaml", + ) + def _model_path(self, data_source: str, name: str) -> str: - return os.path.join(self.models_dir, data_source, f"{name}.yaml") + encoded = self._encoded_model_path(data_source, name) + if os.path.isfile(encoded): + return encoded + if not ( + _is_portable_storage_id(data_source) + and _is_portable_storage_id(name) + ): + return encoded + + raw_ds_dir = os.path.join(self.models_dir, data_source) + exact_ds_dir = _exact_child_path(self.models_dir, data_source) + if exact_ds_dir is None: + if os.path.exists(raw_ds_dir): + return encoded + return os.path.join(raw_ds_dir, f"{name}.yaml") + if not os.path.isdir(exact_ds_dir): + return encoded + raw_path = os.path.join(exact_ds_dir, f"{name}.yaml") + exact_path = _exact_child_path(exact_ds_dir, f"{name}.yaml") + if exact_path is not None: + return exact_path + return encoded if os.path.exists(raw_path) else raw_path + + def _legacy_model_path( + self, data_source: str, name: str, + ) -> str | None: + ds_dir = _exact_child_path(self.models_dir, data_source) + if ds_dir is None or not os.path.isdir(ds_dir): + return None + path = _exact_child_path(ds_dir, f"{name}.yaml") + return path if path is not None and os.path.isfile(path) else None + + def _existing_model_path( + self, data_source: str, name: str, + ) -> str | None: + path = self._model_path(data_source, name) + if ( + path == self._encoded_model_path(data_source, name) + and os.path.isfile(path) + ): + return path + return self._legacy_model_path(data_source, name) # ---- model CRUD -------------------------------------------------------- async def _save_model_impl(self, model: SlayerModel) -> None: - target_dir = os.path.join(self.models_dir, model.data_source) - os.makedirs(target_dir, exist_ok=True) - path = os.path.join(target_dir, f"{model.name}.yaml") + path = self._model_path(model.data_source, model.name) + os.makedirs(os.path.dirname(path), exist_ok=True) data = model.model_dump(mode="json", exclude_none=True) with open(path, "w") as f: yaml.dump(data, f, sort_keys=False) + legacy_path = self._legacy_model_path( + model.data_source, model.name, + ) + if legacy_path is not None and legacy_path != path: + os.remove(legacy_path) async def _list_all_model_identities(self) -> list[tuple[str, str]]: - result: list[tuple[str, str]] = [] + result: set[tuple[str, str]] = set() if not os.path.isdir(self.models_dir): - return result + return [] + + encoded_root = os.path.join(self.models_dir, _ENCODED_IDS_DIR) + if os.path.isdir(encoded_root): + for encoded_ds in os.listdir(encoded_root): + data_source = _decode_storage_id(encoded_ds) + ds_dir = os.path.join(encoded_root, encoded_ds) + if data_source is None or not os.path.isdir(ds_dir): + continue + for filename in os.listdir(ds_dir): + if not filename.endswith(_YAML_EXTS): + continue + name = _decode_storage_id(filename.rsplit(".", 1)[0]) + if name is not None: + result.add((data_source, name)) + for ds in sorted(os.listdir(self.models_dir)): + if ds == _ENCODED_IDS_DIR: + continue ds_dir = os.path.join(self.models_dir, ds) if not os.path.isdir(ds_dir): continue for filename in sorted(os.listdir(ds_dir)): - if filename.endswith((".yaml", ".yml")): - result.append((ds, filename.rsplit(".", 1)[0])) - return result + if filename.endswith(_YAML_EXTS): + result.add((ds, filename.rsplit(".", 1)[0])) + return sorted(result) async def get_model( self, @@ -299,8 +547,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._existing_model_path(data_source, name) + if path is None: # NOSONAR(S6549) — name/data_source were sanitized by _resolve_target_or_none above (rejects '..', path separators, NULs); SlayerModel Pydantic validators sanitize the save path return None try: with open(path) as f: @@ -319,11 +567,15 @@ 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 + deleted = False + for path in ( + self._model_path(data_source, name), + self._legacy_model_path(data_source, name), + ): + if path is not None and os.path.isfile(path): + os.remove(path) + deleted = True + return deleted async def update_column_sampled( self, @@ -335,8 +587,8 @@ async def update_column_sampled( sampled_values: list[str] | None, distinct_count: int | None, ) -> None: - path = self._model_path(data_source, model_name) - if not os.path.exists(path): + path = self._existing_model_path(data_source, model_name) + if path is None: raise ValueError( f"update_column_sampled: model {model_name!r} in datasource " f"{data_source!r} not found." @@ -363,17 +615,39 @@ async def update_column_sampled( # ---- datasource CRUD --------------------------------------------------- + def _datasource_path(self, name: str) -> str: + return _storage_file_path( + self.datasources_dir, name, ".yaml", + ) + + def _legacy_datasource_path(self, name: str) -> str | None: + path = _exact_child_path(self.datasources_dir, f"{name}.yaml") + return path if path is not None and os.path.isfile(path) else None + + def _existing_datasource_path(self, name: str) -> str | None: + path = self._datasource_path(name) + encoded = _encoded_file_path( + self.datasources_dir, name, ".yaml", + ) + if path == encoded and os.path.isfile(path): + return path + return self._legacy_datasource_path(name) + async def save_datasource(self, datasource: DatasourceConfig) -> None: - path = os.path.join(self.datasources_dir, f"{datasource.name}.yaml") + path = self._datasource_path(datasource.name) + os.makedirs(os.path.dirname(path), exist_ok=True) data = datasource.model_dump(mode="json", exclude_none=True) with open(path, "w") as f: yaml.dump(data, f, sort_keys=False) + legacy_path = self._legacy_datasource_path(datasource.name) + if legacy_path is not None and legacy_path != path: + os.remove(legacy_path) 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): + path = self._existing_datasource_path(name) + if path is None: return None try: with open(path) as f: @@ -390,18 +664,32 @@ async def get_datasource(self, name: str) -> DatasourceConfig | None: ) from exc async def list_datasources(self) -> list[str]: - result = [] + result: set[str] = set() + encoded_dir = os.path.join( + self.datasources_dir, _ENCODED_IDS_DIR, + ) + if os.path.isdir(encoded_dir): + for filename in os.listdir(encoded_dir): + if not filename.endswith(_YAML_EXTS): + continue + name = _decode_storage_id(filename.rsplit(".", 1)[0]) + if name is not None: + result.add(name) for filename in sorted(os.listdir(self.datasources_dir)): - if filename.endswith((".yaml", ".yml")): - result.append(filename.rsplit(".", 1)[0]) - return result + if filename.endswith(_YAML_EXTS): + result.add(filename.rsplit(".", 1)[0]) + return sorted(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 + deleted = False + for path in ( + self._datasource_path(name), + self._legacy_datasource_path(name), + ): + if path is not None and os.path.isfile(path): + os.remove(path) + deleted = True + return deleted # ---- datasource priority ----------------------------------------------- @@ -453,17 +741,43 @@ def _memory_md_path(self, memory_id: str) -> str: # (CWE-22). The forbidden set includes "/" and "\\", so a valid id is # always a single safe path segment. _validate_memory_id_charset(memory_id) - return os.path.join(self._memories_dir, f"{memory_id}.md") + return _storage_file_path( + self._memories_dir, memory_id, ".md", + ) + + def _legacy_memory_md_path(self, memory_id: str) -> str | None: + _validate_memory_id_charset(memory_id) + path = _exact_child_path(self._memories_dir, f"{memory_id}.md") + return path if path is not None and os.path.isfile(path) else None + + def _existing_memory_md_path(self, memory_id: str) -> str | None: + path = self._memory_md_path(memory_id) + encoded = _encoded_file_path( + self._memories_dir, memory_id, ".md", + ) + if path == encoded and os.path.isfile(path): + return path + return self._legacy_memory_md_path(memory_id) def _memory_ids_on_disk(self) -> list[str]: - """Every id with a ``memories/.md`` file (stems, ``.md`` stripped).""" + """Every id stored in the encoded or legacy per-memory layout.""" if not os.path.isdir(self._memories_dir): return [] - return [ + result: set[str] = set() + encoded_dir = os.path.join(self._memories_dir, _ENCODED_IDS_DIR) + if os.path.isdir(encoded_dir): + for filename in os.listdir(encoded_dir): + if not filename.endswith(".md"): + continue + memory_id = _decode_storage_id(filename[: -len(".md")]) + if memory_id is not None: + result.add(memory_id) + result.update( fname[: -len(".md")] for fname in os.listdir(self._memories_dir) if fname.endswith(".md") - ] + ) + return sorted(result) async def _next_memory_seq(self) -> str: """DEV-1658: next int-shaped id from the ``memories/`` dir stems. @@ -520,17 +834,23 @@ async def save_memory( # noqa: A002 — mirrors the base signature async def _save_memory_row(self, memory: Memory) -> None: with self._memories_file_lock(): - os.makedirs(self._memories_dir, exist_ok=True) + path = self._memory_md_path(memory.id) + os.makedirs(os.path.dirname(path), exist_ok=True) _atomic_write_text( - self._memory_md_path(memory.id), _memory_to_md(memory), + path, _memory_to_md(memory), ) + legacy_path = self._legacy_memory_md_path(memory.id) + if legacy_path is not None and legacy_path != path: + os.remove(legacy_path) async def _get_memory_row(self, memory_id: str) -> Memory | None: # Lock-free read: writes are atomic (temp + os.replace), so a reader # always sees a complete old-or-new file. A concurrent delete between # the check and the open surfaces as FileNotFoundError → treat as # "missing" (return None) rather than crash. - path = self._memory_md_path(memory_id) + path = self._existing_memory_md_path(memory_id) + if path is None: + 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()) @@ -542,7 +862,9 @@ async def _list_memories_rows( ) -> list[Memory]: memories: list[Memory] = [] for mid in self._memory_ids_on_disk(): - path = self._memory_md_path(mid) + path = self._existing_memory_md_path(mid) + if path is None: + continue try: with open(path, encoding="utf-8") as f: # NOSONAR(S7493) — sync I/O in async by design memories.append(_md_to_memory(mid, f.read())) @@ -559,11 +881,15 @@ 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): - return False - os.remove(path) - return True + deleted = False + for path in ( + self._memory_md_path(memory_id), + self._legacy_memory_md_path(memory_id), + ): + if path is not None and os.path.isfile(path): + os.remove(path) + deleted = True + return deleted @contextlib.contextmanager def _memories_file_lock(self) -> Iterator[None]: diff --git a/tests/test_memory_string_ids.py b/tests/test_memory_string_ids.py index d5d6538a..d39063a5 100644 --- a/tests/test_memory_string_ids.py +++ b/tests/test_memory_string_ids.py @@ -14,6 +14,7 @@ import pytest +from slayer.core.models import DatasourceConfig, SlayerModel from slayer.memories.models import Memory from slayer.storage.base import StorageBackend from slayer.storage.sqlite_storage import SQLiteStorage @@ -29,6 +30,41 @@ def storage(request: pytest.FixtureRequest) -> Iterator[StorageBackend]: yield SQLiteStorage(db_path=os.path.join(tmpdir, "test.db")) +# --------------------------------------------------------------------------- +# Case-sensitive entity identity +# --------------------------------------------------------------------------- + + +class TestCaseSensitiveEntityIds: + async def test_datasources(self, storage: StorageBackend) -> None: + await storage.save_datasource( + DatasourceConfig(name="X", type="postgres", host="upper"), + ) + await storage.save_datasource( + DatasourceConfig(name="x", type="postgres", host="lower"), + ) + + assert (await storage.get_datasource("X")).host == "upper" + assert (await storage.get_datasource("x")).host == "lower" + assert await storage.list_datasources() == ["X", "x"] + + async def test_models(self, storage: StorageBackend) -> None: + await storage.save_model( + SlayerModel(name="X", data_source="db", sql_table="upper"), + ) + await storage.save_model( + SlayerModel(name="x", data_source="db", sql_table="lower"), + ) + + assert ( + await storage.get_model("X", data_source="db") + ).sql_table == "upper" + assert ( + await storage.get_model("x", data_source="db") + ).sql_table == "lower" + assert await storage.list_models("db") == ["X", "x"] + + # --------------------------------------------------------------------------- # Memory.id validation # --------------------------------------------------------------------------- @@ -192,6 +228,9 @@ async def test_case_sensitive_ids( lower = await storage.get_memory("x") assert upper.learning == "upper" assert lower.learning == "lower" + assert {memory.id for memory in await storage.list_memories()} == { + "X", "x", + } async def test_zero_user_id_distinct_from_auto( self, storage: StorageBackend, diff --git a/tests/test_storage.py b/tests/test_storage.py index 501160e1..df759c35 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -4,6 +4,7 @@ import tempfile import pytest +import yaml from slayer.core.enums import DataType from slayer.core.models import Column, DatasourceConfig, SlayerModel @@ -74,6 +75,32 @@ async def test_update_model(self, storage: YAMLStorage, sample_model: SlayerMode loaded = await storage.get_model("test_model") assert loaded.description == "Updated description" + async def test_case_sensitive_model_ids_do_not_alias( + self, storage: YAMLStorage, sample_model: SlayerModel + ) -> None: + upper = sample_model.model_copy( + update={"name": "X", "sql_table": "public.upper"}, + ) + lower = sample_model.model_copy( + update={"name": "x", "sql_table": "public.lower"}, + ) + legacy_dir = os.path.join(storage.models_dir, "test_ds") + os.makedirs(legacy_dir) + with open(os.path.join(legacy_dir, "X.yaml"), "w") as f: # NOSONAR(S7493) — test writes a tiny local fixture; sync I/O is intentional + yaml.safe_dump( + upper.model_dump(mode="json", exclude_none=True), f, + ) + storage = YAMLStorage(base_dir=storage.base_dir) + assert not os.path.exists(os.path.join(legacy_dir, "X.yaml")) + await storage.save_model(lower) + + assert ( + await storage.get_model("X", data_source="test_ds") + ).sql_table == "public.upper" + assert ( + await storage.get_model("x", data_source="test_ds") + ).sql_table == "public.lower" + async def test_empty_model_file_raises_clear_error( self, storage: YAMLStorage, sample_model: SlayerModel ) -> None: @@ -130,6 +157,30 @@ async def test_env_var_resolution(self, storage: YAMLStorage, monkeypatch: pytes loaded = await storage.get_datasource("env_ds") assert loaded.host == "resolved-host" + async def test_case_sensitive_datasource_ids_do_not_alias( + self, storage: YAMLStorage, sample_datasource: DatasourceConfig + ) -> None: + upper = sample_datasource.model_copy( + update={"name": "X", "host": "upper"}, + ) + lower = sample_datasource.model_copy( + update={"name": "x", "host": "lower"}, + ) + with open( # NOSONAR(S7493) — test writes a tiny local fixture; sync I/O is intentional + os.path.join(storage.datasources_dir, "X.yaml"), "w", + ) as f: + yaml.safe_dump( + upper.model_dump(mode="json", exclude_none=True), f, + ) + storage = YAMLStorage(base_dir=storage.base_dir) + assert not os.path.exists( + os.path.join(storage.datasources_dir, "X.yaml"), + ) + await storage.save_datasource(lower) + + assert (await storage.get_datasource("X")).host == "upper" + assert (await storage.get_datasource("x")).host == "lower" + async def test_malformed_yaml_raises_valueerror(self, storage: YAMLStorage) -> None: path = os.path.join(storage.datasources_dir, "bad.yaml") with open(path, "w") as f: @@ -158,3 +209,24 @@ async def test_malformed_datasource_does_not_break_list(self, storage: YAMLStora f.write("name: bad\ntype: [unclosed\n") names = await storage.list_datasources() assert "bad" in names + + def test_migration_refuses_divergent_duplicate( + self, tmp_path, + ) -> None: + datasources_dir = tmp_path / "datasources" + encoded_dir = datasources_dir / ".encoded" + encoded_dir.mkdir(parents=True) + (tmp_path / "models").mkdir() + (tmp_path / "memories").mkdir() + (datasources_dir / "x.yaml").write_text( + "name: x\ntype: postgres\nhost: raw\n", + ) + (encoded_dir / "78.yaml").write_text( + "name: x\ntype: postgres\nhost: encoded\n", + ) + + with pytest.raises(ValueError, match="different content"): + YAMLStorage(base_dir=str(tmp_path)) + + assert (datasources_dir / "x.yaml").exists() + assert (encoded_dir / "78.yaml").exists() diff --git a/tests/test_yaml_memories_mdfiles.py b/tests/test_yaml_memories_mdfiles.py index 8f594f67..ac210388 100644 --- a/tests/test_yaml_memories_mdfiles.py +++ b/tests/test_yaml_memories_mdfiles.py @@ -19,7 +19,7 @@ from slayer.core.errors import MemoryNotFoundError from slayer.core.query import SlayerQuery from slayer.memories.models import Memory -from slayer.storage.yaml_storage import YAMLStorage +from slayer.storage.yaml_storage import YAMLStorage, _memory_to_md @pytest.fixture @@ -295,6 +295,40 @@ async def test_migration_preserves_content(self, base_dir: str) -> None: assert got.description == "preview" assert got.query == q + async def test_migration_preserves_case_sensitive_ids( + self, base_dir: str, + ) -> None: + rows = [ + Memory(id="X", learning="upper", entities=[]).model_dump( + mode="json", + ), + Memory(id="x", learning="lower", entities=[]).model_dump( + mode="json", + ), + ] + self._write_legacy(base_dir, rows) + storage = YAMLStorage(base_dir=base_dir) + + assert (await storage.get_memory("X")).learning == "upper" + assert (await storage.get_memory("x")).learning == "lower" + assert storage._memory_md_path("X") != storage._memory_md_path("x") + + async def test_open_migrates_nonportable_per_file_id( + self, base_dir: str, + ) -> None: + mem_dir = os.path.join(base_dir, "memories") + os.makedirs(mem_dir, exist_ok=True) + legacy_path = os.path.join(mem_dir, "X.md") + with open(legacy_path, "w") as f: # NOSONAR(S7493) — test writes a tiny local fixture; sync I/O is intentional + f.write(_memory_to_md( + Memory(id="X", learning="upper", entities=[]), + )) + + storage = YAMLStorage(base_dir=base_dir) + + assert not os.path.exists(legacy_path) + assert (await storage.get_memory("X")).learning == "upper" + def test_second_construction_is_noop(self, base_dir: str) -> None: rows = [Memory(id="1", learning="x", entities=[] ).model_dump(mode="json")]