Fix YAML storage identity collisions#251
Conversation
📝 WalkthroughWalkthroughYAMLStorage now uses reversible encoded filesystem paths for non-portable identifiers, migrates legacy identity files, and updates model, datasource, and memory CRUD operations to preserve case-sensitive IDs. Documentation and regression tests describe the layouts and migration behavior. ChangesYAML identity storage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant YAMLStorage
participant Filesystem
participant IdentityReader
YAMLStorage->>Filesystem: scan raw and encoded identity paths
Filesystem-->>YAMLStorage: matching identity files
YAMLStorage->>IdentityReader: compare duplicate file contents
IdentityReader-->>YAMLStorage: identical or divergent result
YAMLStorage->>Filesystem: atomically migrate legacy paths
YAMLStorage->>Filesystem: read or write encoded identity paths
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
slayer/storage/yaml_storage.py (3)
81-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant exception in the tuple.
UnicodeDecodeErroris a subclass ofValueError, so catching both is redundant and trips SonarCloud.except ValueErroralone covers bothbytes.fromhex(odd length / bad char) and.decode("utf-8")failures.♻️ Simplify
try: return bytes.fromhex(value).decode("utf-8") - except (UnicodeDecodeError, ValueError): + except ValueError: return None🤖 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/yaml_storage.py` around lines 81 - 88, In _decode_storage_id, simplify the exception handler to catch only ValueError, preserving the existing None return behavior for invalid hex input and UTF-8 decoding failures.Source: Linters/SAST tools
165-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCognitive complexity gate failing (26 > 15). SonarCloud flags
migrate_identity_pathsas a build failure. The three near-identical scan loops (datasources / models / memories) can be factored into a small helper that yields(source, target)pairs given a directory, suffix, and a target-builder, which both clears the gate and removes duplication.🤖 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/yaml_storage.py` around lines 165 - 226, Reduce the cognitive complexity of migrate_identity_paths by extracting the repeated datasource, model, and memory scanning logic into a small helper that yields source/target pairs from a directory, suffix, and target-builder. Update migrate_identity_paths to use this helper while preserving the existing portable-ID checks, encoded target paths, and _move_identity_files behavior.Sources: Linters/SAST tools, Pipeline failures
511-539: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCognitive complexity gate failing (29 > 15). SonarCloud marks
_list_all_model_identitiesas a build failure. Extracting the encoded-namespace scan and the raw per-datasource scan into two small helpers that each returnset[tuple[str, str]]would satisfy the gate without changing behavior.🤖 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/yaml_storage.py` around lines 511 - 539, The cognitive complexity is concentrated in _list_all_model_identities. Extract the encoded namespace traversal and raw datasource traversal into separate small helpers returning set[tuple[str, str]], then have _list_all_model_identities handle directory validation, invoke both helpers, combine their results, and return the sorted identities without changing filtering or decoding behavior.Sources: Linters/SAST tools, Pipeline failures
🤖 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.
Nitpick comments:
In `@slayer/storage/yaml_storage.py`:
- Around line 81-88: In _decode_storage_id, simplify the exception handler to
catch only ValueError, preserving the existing None return behavior for invalid
hex input and UTF-8 decoding failures.
- Around line 165-226: Reduce the cognitive complexity of migrate_identity_paths
by extracting the repeated datasource, model, and memory scanning logic into a
small helper that yields source/target pairs from a directory, suffix, and
target-builder. Update migrate_identity_paths to use this helper while
preserving the existing portable-ID checks, encoded target paths, and
_move_identity_files behavior.
- Around line 511-539: The cognitive complexity is concentrated in
_list_all_model_identities. Extract the encoded namespace traversal and raw
datasource traversal into separate small helpers returning set[tuple[str, str]],
then have _list_all_model_identities handle directory validation, invoke both
helpers, combine their results, and return the sorted identities without
changing filtering or decoding behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87312380-16b6-485e-b8ff-f3d7beab7911
📒 Files selected for processing (8)
docs/concepts/memories.mddocs/concepts/models.mddocs/configuration/datasources.mddocs/configuration/storage.mdslayer/storage/yaml_storage.pytests/test_memory_string_ids.pytests/test_storage.pytests/test_yaml_memories_mdfiles.py
|
#249 is solved in #252 – the fix we did goes the other route, which is failing loudly on colliding filenames when using YAMLStorage, on every platform. We chose in favor of that solution because the entire purpose of YAMLStorage is to make the models easy to explore and navigate in the filesystem, and encoding filenames would defeat this. |
2 participants
Summary
.encodednamespace using reversible UTF-8 hex while retaining readable paths for portable lowercase IDs.xto an existingXfile.Xandx, legacy migration regressions, and storage-layout documentation.Closes #249
Validation
.venv/bin/ruff check slayer/ tests/.venv/bin/pytest tests/ -m "not integration" --timeout=120 -q— 6,501 passed, 4 skipped, 3 xfailed.venv/bin/pytest tests/ -m "integration" --timeout=900 -q— passed with expected environment-specific skips and xfailSummary by CodeRabbit