Reject case-colliding ids across all storage backends#252
Conversation
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
📝 WalkthroughWalkthroughChangesCase-collision persistence safeguards
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Interface
participant StorageBackend
participant Storage
Client->>Interface: create datasource or ingest models
Interface->>StorageBackend: save entity
StorageBackend->>Storage: validate identity and persist
Storage-->>StorageBackend: IdCollisionError on case-only collision
StorageBackend-->>Interface: validation failure
Interface-->>Client: HTTP 400 or per-item failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
slayer/storage/join_sync.py (1)
259-282: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDelegate
save_memoryto the inner backend
JoinSyncStoragestill needs a publicsave_memoryoverride that forwards toself._inner.save_memory(...). The inherited template splits the collision check from the write and bypassesSQLiteStorage.save_memory’s transaction, even though the low-level SQLite helpers keepmemory_entitiesin sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/storage/join_sync.py` around lines 259 - 282, Add a public save_memory override to JoinSyncStorage that forwards the memory and any supported arguments directly to self._inner.save_memory(...). Place it alongside the existing memory delegation methods, ensuring calls use SQLiteStorage.save_memory’s transactional implementation instead of the inherited template.
🧹 Nitpick comments (3)
slayer/storage/sqlite_storage.py (1)
439-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
"SELECT id FROM memories"literal (SonarCloud S1192).The same literal appears 3 times (here,
_next_memory_seq_sync_from_conn,_list_memory_ids_sync). Extract a module-level constant to avoid drift between the three query sites.♻️ Proposed fix
+_SELECT_MEMORY_IDS_SQL = "SELECT id FROM memories" + ... - ids = [ - row[0] if isinstance(row[0], str) else str(row[0]) - for row in conn.execute("SELECT id FROM memories") - ] + ids = [ + row[0] if isinstance(row[0], str) else str(row[0]) + for row in conn.execute(_SELECT_MEMORY_IDS_SQL) + ]Also applies to: 494-505, 562-569
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/storage/sqlite_storage.py` around lines 439 - 452, Extract the repeated "SELECT id FROM memories" SQL into a module-level constant, then update the query in the transaction case-collision check and the corresponding queries in _next_memory_seq_sync_from_conn and _list_memory_ids_sync to use that constant.Source: Linters/SAST tools
slayer/storage/base.py (1)
126-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_find_case_colliding_idis called positionally everywhere; guideline requires keyword args for multi-param functions.Every call site (
_check_model_identity_collisionlines 236/242,save_datasourceline 465,save_memoryline 702, andsqlite_storage.pyline 446) passescandidate/existingpositionally. As per coding guidelines, "Use keyword arguments for functions with more than one parameter." Consider marking the params keyword-only (*) to enforce this at every call site.♻️ Proposed fix
def _find_case_colliding_id( - candidate: str, existing: Iterable[str], + *, candidate: str, existing: Iterable[str], ) -> str | None:Then update each call site to
_find_case_colliding_id(candidate=..., existing=...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/storage/base.py` around lines 126 - 141, Make _find_case_colliding_id parameters keyword-only by adding the separator before candidate, then update every call in _check_model_identity_collision, save_datasource, save_memory, and sqlite_storage.py to pass candidate= and existing= explicitly. Preserve the function’s collision behavior unchanged.Source: Path instructions
docs/configuration/storage.md (1)
85-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: "enforced in the shared base class" is imprecise for the SQLite memory path.
For SQLite, memory-id collision checking is actually reimplemented locally in
_save_memory_atomic_sync(for transactional atomicity), not routed through the shared base-class check used by models/datasources. Worth a one-clause caveat if you want full precision, but this doesn't materially mislead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/configuration/storage.md` around lines 85 - 86, Update the SQLite case-sensitivity documentation to clarify that collision enforcement uses the shared base class for models and datasources, while the SQLite memory path reimplements the check locally in _save_memory_atomic_sync for transactional atomicity. Preserve the existing export and cross-platform migration guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@slayer/storage/base.py`:
- Around line 453-470: Make the collision check and persistence in
save_datasource and save_model atomic for SQLite by executing both under the
backend’s existing BEGIN IMMEDIATE transaction pattern. Keep the list/identity
checks and corresponding _save_datasource_impl or model-save implementation
within the same transaction, so concurrent case-variant saves cannot both pass
validation; preserve the existing memory-path behavior.
---
Outside diff comments:
In `@slayer/storage/join_sync.py`:
- Around line 259-282: Add a public save_memory override to JoinSyncStorage that
forwards the memory and any supported arguments directly to
self._inner.save_memory(...). Place it alongside the existing memory delegation
methods, ensuring calls use SQLiteStorage.save_memory’s transactional
implementation instead of the inherited template.
---
Nitpick comments:
In `@docs/configuration/storage.md`:
- Around line 85-86: Update the SQLite case-sensitivity documentation to clarify
that collision enforcement uses the shared base class for models and
datasources, while the SQLite memory path reimplements the check locally in
_save_memory_atomic_sync for transactional atomicity. Preserve the existing
export and cross-platform migration guidance.
In `@slayer/storage/base.py`:
- Around line 126-141: Make _find_case_colliding_id parameters keyword-only by
adding the separator before candidate, then update every call in
_check_model_identity_collision, save_datasource, save_memory, and
sqlite_storage.py to pass candidate= and existing= explicitly. Preserve the
function’s collision behavior unchanged.
In `@slayer/storage/sqlite_storage.py`:
- Around line 439-452: Extract the repeated "SELECT id FROM memories" SQL into a
module-level constant, then update the query in the transaction case-collision
check and the corresponding queries in _next_memory_seq_sync_from_conn and
_list_memory_ids_sync to use that constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0af37df7-0ea6-446f-9710-556c7866a3a0
📒 Files selected for processing (18)
CLAUDE.mddocs/concepts/ingestion.mddocs/concepts/memories.mddocs/concepts/models.mddocs/configuration/storage.mdslayer/api/server.pyslayer/cli.pyslayer/core/errors.pyslayer/mcp/server.pyslayer/storage/base.pyslayer/storage/join_sync.pyslayer/storage/sqlite_storage.pyslayer/storage/v4_migration.pyslayer/storage/yaml_storage.pytests/test_api_server.pytests/test_id_collision.pytests/test_memory_string_ids.pytests/test_yaml_exact_entry.py
| 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/<ds>/`` 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) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does asyncio.to_thread yield control to the event loop, allowing interleaving between two separate to_thread calls?
💡 Result:
Yes, asyncio.to_thread yields control to the event loop, allowing for interleaving [1][2]. When you call await asyncio.to_thread(...), the current coroutine suspends and yields control back to the event loop [1]. While the function submitted to to_thread is executing in a separate thread (via a ThreadPoolExecutor), the event loop remains free to run other tasks, callbacks, or perform I/O operations [1][2]. Regarding the interleaving of two separate to_thread calls: 1. If you await each to_thread call sequentially (e.g., await asyncio.to_thread(func1); await asyncio.to_thread(func2)), the second call will not start until the first has finished because the event loop is blocked at the first await expression [3]. 2. If you want two separate to_thread calls to run concurrently and interleave, you must schedule them as concurrent tasks, such as by using asyncio.gather or by creating tasks with asyncio.create_task [3]. This allows the event loop to manage both operations simultaneously while waiting for their completion in separate threads [3]. In essence, asyncio.to_thread is a non-blocking awaitable that allows you to offload blocking synchronous code to a separate thread without stalling the event loop, provided that you properly schedule the calls to run concurrently [1][2][3].
Citations:
- 1: https://stackoverflow.com/questions/65316863/is-asyncio-to-thread-method-different-to-threadpoolexecutor
- 2: https://docs.python.org/3/library/asyncio-task.html
- 3: https://stackoverflow.com/questions/75392320/blocking-function-sent-to-thread-not-executing-if-awaited-secondly
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== base.py outline ==\n'
ast-grep outline slayer/storage/base.py --view expanded | sed -n '1,220p'
printf '\n== sqlite_storage.py outline ==\n'
ast-grep outline slayer/storage/sqlite_storage.py --view expanded | sed -n '1,260p'
printf '\n== relevant save/collision methods ==\n'
rg -n "async def (save_datasource|save_model|_save_datasource_impl|_save_model_impl|_list_all_model_identities|list_datasources|_check_model_identity_collision|_save_memory_atomic_sync|BEGIN IMMEDIATE)" slayer/storage/base.py slayer/storage/sqlite_storage.pyRepository: MotleyAI/slayer
Length of output: 7188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [Path('slayer/storage/base.py'), Path('slayer/storage/sqlite_storage.py')]:
print(f"\n### {p} ###")
text = p.read_text()
for needle in [
'async def save_datasource',
'async def save_model',
'async def _save_datasource_impl',
'async def _save_model_impl',
'async def _list_all_model_identities',
'async def list_datasources',
'async def _check_model_identity_collision',
'BEGIN IMMEDIATE',
'asyncio.to_thread',
'asyncio.Lock',
]:
if needle in text:
print(f"FOUND {needle}")
PYRepository: MotleyAI/slayer
Length of output: 674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' slayer/storage/base.py | nl -ba | sed -n '1,260p'
printf '\n----\n'
sed -n '1,260p' slayer/storage/sqlite_storage.py | nl -ba | sed -n '1,260p'Repository: MotleyAI/slayer
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== base.py relevant excerpts ==\n'
sed -n '200,280p' slayer/storage/base.py
printf '\n----\n'
sed -n '453,490p' slayer/storage/base.py
printf '\n====\n'
printf '\n== sqlite_storage.py relevant excerpts ==\n'
sed -n '186,380p' slayer/storage/sqlite_storage.py
printf '\n----\n'
sed -n '404,540p' slayer/storage/sqlite_storage.py
printf '\n----\n'
sed -n '540,640p' slayer/storage/sqlite_storage.pyRepository: MotleyAI/slayer
Length of output: 22447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== sqlite_storage init/schema excerpts ==\n'
sed -n '133,185p' slayer/storage/sqlite_storage.py
printf '\n== schema-related searches ==\n'
rg -n "CREATE TABLE|CREATE UNIQUE|UNIQUE INDEX|COLLATE NOCASE|PRIMARY KEY|models \\(|datasources \\(" slayer/storage/sqlite_storage.py slayer/storage/*.pyRepository: MotleyAI/slayer
Length of output: 5760
Make the case-collision guard atomic on SQLite
save_model and save_datasource can still race across the asyncio.to_thread boundaries: SQLite’s PRIMARY KEY here is case-sensitive, so concurrent saves like Foo/foo can both pass the check and persist. The memory path already uses BEGIN IMMEDIATE; models/datasources need the same protection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@slayer/storage/base.py` around lines 453 - 470, Make the collision check and
persistence in save_datasource and save_model atomic for SQLite by executing
both under the backend’s existing BEGIN IMMEDIATE transaction pattern. Keep the
list/identity checks and corresponding _save_datasource_impl or model-save
implementation within the same transaction, so concurrent case-variant saves
cannot both pass validation; preserve the existing memory-path behavior.
1 participant
Closes #249.
Problem
YAMLStorageuses raw logical ids as filenames, so ids differing only by case (Xvsx) address the same file on case-insensitive filesystems (macOS / Windows defaults) and silently overwrite each other — datasources, models, and memories alike. Reads and deletes were equally unsafe (get_model("X")returned modelx;delete_model("X")removedx.yaml), and the legacy memory migration could collapse colliding records and then deletememories.yaml.SQLiteStoragekept case variants distinct, so the two backends silently diverged.Fix: fail loudly, uniformly
Instead of the reversible filename encoding suggested in the issue, saving an id that differs only by case (
casefold()-equal, raw-different) from an existing one now raises a newIdCollisionError(SlayerError, ValueError)— enforced in theStorageBackendbase templates, so every backend rejects on every platform and stores stay portable across backends and OSes. Exact-id re-saves remain upserts; a legacy store already holding a colliding pair raises on a save of either spelling, with a rename-or-delete remediation.save_datasourcebecomes a base template method delegating to a new abstract_save_datasource_impl(same pattern assave_model/_save_model_impl).save_memorychecks via a new cheap_list_memory_ids()primitive (YAML: one listdir; SQLite:SELECT id; JoinSync: delegation).SQLiteStorage's atomicsave_memoryoverride bypasses the base template, so the check is duplicated inside itsBEGIN IMMEDIATEtransaction.data_sourceagainst registered datasource names ∪ other models' data_sources (they share themodels/<ds>/directory namespace), andnamewithin the same datasource.save_model(_validate=False)(the migration write-back) bypasses the check so legacy data stays loadable.YAML read/delete exact-name verification
YAML lookups and deletes now compare the exact entry against
os.listdir(_exact_entry_exists) instead ofos.path.exists/bareopen, so on macOS a case-variantget_model/get_datasource/get_memoryreturns not-found and a delete is a no-op — instead of silently reading or removing the wrong entity in a store copied over from a case-sensitive system.Migration pre-checks
migrate_memories_layoutandmigrate_yaml_layout(restructured to plan-then-move) refuse to run when targets would collide by case, before writing anything — legacy files preserved for manual repair.Surfacing
REST
POST /datasources→ 400 (model/memory endpoints already mappedValueError→ 400); MCPcreate_datasourcereturns a friendly message and its auto-ingest loop reports per-model save failures without aborting (quoted case-variant tables like"Orders"/orders: first wins, second reported); CLI prints-and-exits on datasource create, skips-and-continues in ingest loops.ingest_datasource_idempotentalready isolated per-model errors intoIngestionResult.errors.Tests
test_case_sensitive_ids(which pinned the old divergent behavior and only passed on case-sensitive CI filesystems) inverted totest_case_colliding_ids_rejected.tests/test_id_collision.py: helpers, all three entity kinds on both backends, cross-namespace datasource/model checks, legacy colliding pairs seeded via raw SQL, theresolve_storage(JoinSync-wrapped) production path,_validate=Falsebypass, migration refusals.tests/test_yaml_exact_entry.py: read/delete not-found semantics for case-variant lookups (assertions hold on both case-sensitive and case-insensitive dev machines).Known scope choice: the key is
casefold()only — slightly broader than ASCII case (ß≡ss); Unicode-normalization aliasing (NFC vs NFD) is deliberately out of scope.https://claude.ai/code/session_011sbQKNXcUtGAsCXtmJXHvR
Summary by CodeRabbit
Bug Fixes
API & CLI
Documentation