fix(openclaw): remove clear-all's disk-storage handling - #500
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughClear-all no longer enumerates legacy disk storage targets. Unsupported disk configurations now raise ChangesClear-all storage handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Legacy disk configurations are invalid and are safely refused rather than being guessed as SQLite targets. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
#499 flagged `_disk_org_targets` as looking like the same class of bug it fixed -- it globs every `disk_*` child rather than the caller's -- but left it alone for want of a convention to verify a narrowing against. There is none, because there is no backend: #98 removed disk storage entirely on 2026-05-28, in its own words a "clean cold deletion (no deprecation window)". The plugin was never updated, so this handling has outlived what it served by three and a half months. Nothing can reach it. `StorageConfig` is a union of SQLite, Supabase, Postgres and ManagedSupabase -- no disk variant -- and `_STORAGE_CONFIG_ADAPTER` rejects both `{"type": "disk", ...}` and a bare `{"dir_path": ...}`. The one surviving `dir_path` reference in the package, in `_storage_labels.py`, is guarded on `cls_name == "StorageConfigLocal"`, a class that no longer exists either. Removed: `_disk_org_targets`, the `kind == "disk"` branch in target resolution, and both legacy shapes in `_storage_config_kind`. No disk logic remains. A leftover disk config now falls through to the refusal the function already has for shapes it cannot interpret. That is deliberate rather than incidental: `validate_stored_config` rejects those configs, so the server cannot load such an org either. We do not know what storage it uses, and a destructive command must not guess. Translating `disk` to `sqlite` was considered and rejected. It asserts something false -- a disk config is not a SQLite config -- and would delete a database on the strength of that guess, while keeping a removed backend alive under another name. The reasoning behind it was also wrong: `resolve_storage_backend`'s fallback to SQLite applies to the `REFLEXIO_STORAGE` environment variable, not to a persisted `storage_config`, which is simply invalid. Test: both legacy shapes refuse and delete nothing. Reinstating the `disk` -> `sqlite` mapping fails it.
guangyu-reflexio
force-pushed
the
fix/drop-vestigial-disk-clear-all
branch
from
September 12, 2026 05:28
6dfd712 to
fd64395
Compare
guangyu-reflexio
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #499, which flagged this and deliberately did not touch it.
Answering the question #499 left open
#499 noted that
_disk_org_targetsglobs everydisk_*child rather than the caller's — the same shape as the bug it fixed — but left it alone, because there was no convention to verify a narrowing against.There is no convention because there is no backend. #98 removed disk storage entirely on 2026-05-28, describing itself as a "clean cold deletion (no deprecation window)". The plugin was never updated to match, so this handling has outlived what it served by three and a half months. The answer was delete, not narrow.
It is unreachable
StorageConfigis a union ofStorageConfigSQLite | StorageConfigSupabase | StorageConfigPostgres | StorageConfigManagedSupabase | None. No disk variant._STORAGE_CONFIG_ADAPTERrejects both legacy shapes outright — verified directly:The only surviving
dir_pathreference in the package, in_storage_labels.py, is guarded oncls_name == "StorageConfigLocal"— a class that no longer exists either. (Left alone here; inert, and outside this file's scope.)No
DiskStorageimplementation exists anywhere in either package.What is removed
_disk_org_targetskind == "disk"branch in_resolve_clear_all_targets_storage_config_kindNo disk logic remains. The one surviving mention of the word is a comment explaining why the fallthrough below is correct.
Leftover configs refuse, rather than being reinterpreted
A config left over from the disk backend now falls through to the refusal
_storage_config_kindalready has for shapes it cannot interpret:That is deliberate.
validate_stored_configrejects those configs, so the server cannot load such an org either — we genuinely do not know what storage it uses, and a destructive command must not guess.An earlier revision of this PR mapped
diskontosqliteinstead. That was wrong on two counts and has been removed:resolve_storage_backend's fallback to SQLite, but that fallback applies to theREFLEXIO_STORAGEenvironment variable, not to a persistedstorage_config. Different input, different path; the config is simply invalid.Test Plan
Local, as Actions has no budget:
ruff check+ruff format --check: cleanpyright: 0 errors, 0 warningsNew test, parametrized over both legacy shapes: each refuses and deletes nothing.
Mutation-tested: reinstating the
disk→sqlitemapping fails theexplicit-typecase. Mutation confirmed present before running; file restored by checksum afterwards.Summary by CodeRabbit
clear-allnow reports unsupported legacy disk storage configurations instead of attempting to remove them.