From 2fd19bddfc4b951576c9e91bfd7bc3f06ca692a0 Mon Sep 17 00:00:00 2001 From: guangyu-reflexio Date: Fri, 11 Sep 2026 18:10:01 -0700 Subject: [PATCH] fix(openclaw): scope clear-all to the calling dataset identity `_resolve_clear_all_targets` returned the whole storage root as one directory target, then `rmtree`'d it. Everything after that in the function scopes per identity -- the `disk` branch even enumerates children individually -- but it is all moot, because the first target already swallows the directory those children live in. Dataset isolation made this consequential rather than merely untidy. The root used to hold one shared `reflexio.db`; it now holds one `reflexio_.db` per identity, so clearing one identity destroyed every other identity's database. Two local backends under distinct `REFLEXIO_DEFAULT_ORG_ID` values is not an exotic setup -- `default_get_org_id` documents it as the supported way to stop claude-smart and the self-host backend fighting over one config. It also contradicted `derive_db_path`, which promises a flat filename precisely so "sibling artifacts in the same directory -- the enterprise `sql_app.db`, the `disk_*` trees -- are untouched". They were not untouched. Now `clear-all` enumerates what the calling identity owns: its `reflexio_.db` plus SQLite's sidecars, and the legacy `reflexio.db` only when the `_dataset_identity` claim says it is ours (or when nobody has claimed it, in which case we are the installation that would adopt it on next start). Everything else in the root survives. Resolution is strictly read-only. `resolve_sqlite_db_path` upstream is not reusable here: it mkdirs the root and writes a claim row, so it would create a database in order to delete one. Two tests pin that no file or directory is created during resolution. The filename derivation is duplicated rather than imported. Reaching `_dataset_path` executes the storage package's `__init__` and costs ~1.7s measured, and `openclaw-smart-hook` runs per session event. `test_derived_filename_matches_the_canonical_resolver` imports the canonical implementation (tests can afford what the CLI cannot) and asserts the two agree, so the duplication cannot drift silently. Tests: `_resolve_clear_all_targets` had none -- all five existing `clear-all` tests patch it out and exercise only the wrapper, which is how this survived. The new cases build a real root on disk and assert what SURVIVES after removal actually runs. Asserting a path is merely absent from the target list would have passed against the bug too, since the root target destroys files it never names. `_disk_org_targets` is deliberately untouched: it globs every `disk_*` child rather than the caller's, which looks like the same class of bug, but no disk backend exists in this package and nothing here produces those directories, so there is no convention to verify a narrowing against. --- reflexio/integrations/openclaw/README.md | 2 +- .../openclaw/plugin/src/openclaw_smart/cli.py | 141 +++++++++++- .../plugin/tests/test_clear_all_targets.py | 212 ++++++++++++++++++ 3 files changed, 345 insertions(+), 10 deletions(-) create mode 100644 reflexio/integrations/openclaw/plugin/tests/test_clear_all_targets.py diff --git a/reflexio/integrations/openclaw/README.md b/reflexio/integrations/openclaw/README.md index b4d83c06b..ee6a89a61 100644 --- a/reflexio/integrations/openclaw/README.md +++ b/reflexio/integrations/openclaw/README.md @@ -91,7 +91,7 @@ Six skill folders ship under `plugin/skills/` (`reflexio` is the always-on contr | `show` | Dump currently-known skills + preferences for this project as markdown | | `dashboard` | Open the local reflexio web UI (`http://localhost:3001`, shared with claude-smart) | | `restart` | Restart the local reflexio backend cleanly | -| `clear-all` | Delete all locally-stored skills + preferences (destructive, prompts) | +| `clear-all` | Delete this dataset's locally-stored skills + preferences (destructive, prompts) | ## Configuration diff --git a/reflexio/integrations/openclaw/plugin/src/openclaw_smart/cli.py b/reflexio/integrations/openclaw/plugin/src/openclaw_smart/cli.py index caf29ecd9..fb7df2f9b 100644 --- a/reflexio/integrations/openclaw/plugin/src/openclaw_smart/cli.py +++ b/reflexio/integrations/openclaw/plugin/src/openclaw_smart/cli.py @@ -22,6 +22,7 @@ import json import os import shutil +import sqlite3 import subprocess import sys import time @@ -48,6 +49,20 @@ _DEFAULT_STORAGE_ROOT = _REFLEXIO_DIR / "data" _REFLEXIO_CONFIG_PATH = _REFLEXIO_DIR / "configs" / "config_self-host-org.json" _LOCAL_STORAGE_ENV = "LOCAL_STORAGE_PATH" +_DEFAULT_ORG_ID_ENV = "REFLEXIO_DEFAULT_ORG_ID" + +# Mirrors reflexio.cli.bootstrap_config._DEFAULT_ORG_ID. +_DEFAULT_ORG_ID = "self-host-org" + +# Mirrors sqlite_storage._dataset_path. Duplicated rather than imported: that +# module is cheap on its own, but reaching it executes the storage package's +# __init__ and costs ~1.7s, and `openclaw-smart-hook` runs per session event. +# `test_derived_filename_matches_the_canonical_resolver` pins the two together. +_LEGACY_DB_FILENAME = "reflexio.db" +_IDENTITY_CLAIM_TABLE = "_dataset_identity" + +# Files SQLite keeps beside the database it is given. +_SQLITE_SIDECAR_SUFFIXES = ("", "-wal", "-shm", "-journal") def _latest_session_id() -> str | None: @@ -410,11 +425,124 @@ def _disk_org_targets(base_dir: Path) -> list[_ClearAllTarget]: ] -def _resolve_clear_all_targets() -> list[_ClearAllTarget]: - targets = [ - _ClearAllTarget(_effective_storage_root(), "dir", "managed local storage root") +def _derive_db_filename(org_id: str) -> str: + """Return the database filename *org_id* owns. + + Args: + org_id (str): The dataset identity. + + Returns: + str: The filename, e.g. ``reflexio_self-host-org.db``. + """ + return f"reflexio_{org_id}.db" + + +def _effective_org_id() -> str: + """Resolve the dataset identity this installation reads and writes. + + Same precedence the server and ``reset_db.py`` use, so ``clear-all`` clears + the database the backend would actually open: ``REFLEXIO_DEFAULT_ORG_ID`` + from the environment, then from ``~/.reflexio/.env``, then the default. + + Returns: + str: The dataset identity. + """ + raw = os.environ.get(_DEFAULT_ORG_ID_ENV, "").strip() + if not raw: + raw = _read_dotenv_value(_REFLEXIO_ENV_PATH, _DEFAULT_ORG_ID_ENV) or "" + return raw.strip() or _DEFAULT_ORG_ID + + +def _claimed_identity(path: Path) -> str | None: + """Return the identity that claims *path*, if it records one. + + Strictly read-only -- opened ``mode=ro`` so inspecting a database never + creates one, and never takes the write lock a running backend may hold. + + Args: + path (Path): The database to inspect. + + Returns: + str | None: The claiming identity, or ``None`` when the file is absent, + unreadable, or carries no claim. + """ + if not path.is_file(): + return None + try: + conn = sqlite3.connect(f"file:{path}?mode=ro", uri=True) + except sqlite3.Error: + return None + try: + row = conn.execute( + f"SELECT org_id FROM {_IDENTITY_CLAIM_TABLE} WHERE k = 1" # noqa: S608 + ).fetchone() + except sqlite3.Error: + return None + finally: + conn.close() + return str(row[0]) if row and row[0] else None + + +def _sqlite_artifact_targets(db_path: Path, label: str) -> list[_ClearAllTarget]: + """The database plus the sidecars SQLite keeps beside it.""" + return [ + _ClearAllTarget(Path(f"{db_path}{suffix}"), "file", label) + for suffix in _SQLITE_SIDECAR_SUFFIXES ] + +def _identity_owned_targets(root: Path, org_id: str) -> list[_ClearAllTarget]: + """Targets under *root* that belong to *org_id*, and nothing else. + + The storage root is shared: after dataset isolation it holds one + ``reflexio_.db`` per identity, alongside artifacts this plugin does not + own (the enterprise ``sql_app.db``, ``disk_*`` trees). ``derive_db_path`` + documents those siblings as untouched, so enumerate what we own instead of + deleting the directory that contains them. + + Args: + root (Path): The storage root. + org_id (str): The dataset identity being cleared. + + Returns: + list[_ClearAllTarget]: Targets owned by *org_id*. + """ + if not root.is_dir(): + return [] + + targets = _sqlite_artifact_targets( + root / _derive_db_filename(org_id), "this dataset's SQLite data" + ) + + # A pre-isolation install still reads `reflexio.db`, adopted in place by its + # first claimant. Clear it only when it is ours -- or when nobody has + # claimed it yet, in which case we are the installation that would adopt it. + legacy = root / _LEGACY_DB_FILENAME + if legacy.is_file(): + owner = _claimed_identity(legacy) + if owner in (None, org_id): + targets.extend(_sqlite_artifact_targets(legacy, "legacy SQLite data")) + + return targets + + +def _resolve_clear_all_targets() -> list[_ClearAllTarget]: + """Resolve exactly what ``clear-all`` may delete. + + Read-only: resolution never creates the root, a database, or an identity + claim. (``resolve_sqlite_db_path`` upstream deliberately does all three, so + it is not reusable here -- it would create a database in order to delete + one.) + + Returns: + list[_ClearAllTarget]: Deduplicated, validated targets. + + Raises: + _ClearAllError: If storage is remote, misconfigured, or a target is unsafe. + """ + root = _effective_storage_root() + targets = _identity_owned_targets(root, _effective_org_id()) + config = _load_reflexio_config() storage_config = config.get("storage_config") if config else None if storage_config is not None: @@ -433,12 +561,7 @@ def _resolve_clear_all_targets() -> list[_ClearAllTarget]: raw_db_path.strip(), source="configured SQLite db_path" ) targets.extend( - _ClearAllTarget( - Path(f"{db_path}{suffix}"), - "file", - "configured SQLite data", - ) - for suffix in ("", "-wal", "-shm", "-journal") + _sqlite_artifact_targets(db_path, "configured SQLite data") ) elif kind == "disk": raw_dir_path = storage_config.get("dir_path") diff --git a/reflexio/integrations/openclaw/plugin/tests/test_clear_all_targets.py b/reflexio/integrations/openclaw/plugin/tests/test_clear_all_targets.py new file mode 100644 index 000000000..cd7d33f9a --- /dev/null +++ b/reflexio/integrations/openclaw/plugin/tests/test_clear_all_targets.py @@ -0,0 +1,212 @@ +"""Tests for ``clear-all`` target resolution. + +``_resolve_clear_all_targets`` had no coverage: every existing ``clear-all`` +test patches it out and exercises only the command wrapper around it. That is +how it kept returning the whole storage root as one directory target long after +dataset isolation started putting one ``reflexio_.db`` per identity in that +root -- so clearing one identity destroyed every other identity's database, plus +any sibling artifact (the enterprise ``sql_app.db``, the ``disk_*`` trees) that +``derive_db_path`` documents as untouched. + +These tests build a real root on disk and assert what survives, not just what +goes. +""" + +from __future__ import annotations + +import os +import sqlite3 +from pathlib import Path + +import pytest +from openclaw_smart import cli + +OTHER_ORG = "other-org" +OUR_ORG = "self-host-org" + + +@pytest.fixture +def root(monkeypatch, tmp_path) -> Path: + """An isolated storage root, with no reflexio config on disk.""" + storage = tmp_path / "data" + storage.mkdir() + monkeypatch.setenv("LOCAL_STORAGE_PATH", str(storage)) + monkeypatch.setenv("REFLEXIO_DEFAULT_ORG_ID", OUR_ORG) + # _load_reflexio_config() reads a module-level path; point it at nothing so + # these cases exercise the default (unconfigured) branch. + monkeypatch.setattr(cli, "_REFLEXIO_CONFIG_PATH", tmp_path / "absent.json") + monkeypatch.setattr(cli, "_REFLEXIO_ENV_PATH", tmp_path / "absent.env") + return storage + + +def _make_db(path: Path, claimed_by: str | None) -> None: + """Create a SQLite file, optionally carrying a dataset identity claim.""" + conn = sqlite3.connect(path) + try: + conn.execute("CREATE TABLE IF NOT EXISTS junk (x INTEGER)") + if claimed_by is not None: + conn.execute( + "CREATE TABLE IF NOT EXISTS _dataset_identity (" + " k INTEGER PRIMARY KEY CHECK (k = 1)," + " org_id TEXT NOT NULL," + " claimed_at TEXT NOT NULL)" + ) + conn.execute( + "INSERT INTO _dataset_identity (k, org_id, claimed_at)" + " VALUES (1, ?, '2026-01-01T00:00:00Z')", + (claimed_by,), + ) + conn.commit() + finally: + conn.close() + + +def _paths(targets) -> set[Path]: + return {t.path for t in targets} + + +def _clear() -> None: + """Resolve and actually remove, so assertions can look at the filesystem. + + Asserting a path is merely absent from the target list passes against the + bug too -- the root target destroys files it never names. + """ + for target in cli._resolve_clear_all_targets(): + cli._remove_clear_all_target(target) + + +def test_does_not_target_the_storage_root_itself(root): + """The root is shared; deleting it as a directory is the whole bug.""" + _make_db(root / f"reflexio_{OUR_ORG}.db", claimed_by=OUR_ORG) + + targets = cli._resolve_clear_all_targets() + + assert root.resolve() not in _paths(targets) + + +def test_targets_our_database_and_its_sidecars(root): + our_db = root / f"reflexio_{OUR_ORG}.db" + _make_db(our_db, claimed_by=OUR_ORG) + for suffix in ("-wal", "-shm", "-journal"): + Path(f"{our_db}{suffix}").write_text("") + + paths = _paths(cli._resolve_clear_all_targets()) + + assert our_db.resolve() in paths + for suffix in ("-wal", "-shm", "-journal"): + assert Path(f"{our_db}{suffix}").resolve() in paths + + +def test_spares_another_identitys_database(root): + """The regression this file exists for.""" + ours = root / f"reflexio_{OUR_ORG}.db" + _make_db(ours, claimed_by=OUR_ORG) + theirs = root / f"reflexio_{OTHER_ORG}.db" + _make_db(theirs, claimed_by=OTHER_ORG) + + _clear() + + assert theirs.exists(), "another identity's database was destroyed" + assert not ours.exists(), "our own database should still be cleared" + + +def test_spares_sibling_artifacts_in_the_same_root(root): + """``derive_db_path`` promises these are untouched; honor that here too.""" + _make_db(root / f"reflexio_{OUR_ORG}.db", claimed_by=OUR_ORG) + enterprise = root / "sql_app.db" + _make_db(enterprise, claimed_by=None) + stray = root / "notes.txt" + stray.write_text("keep me") + + _clear() + + assert enterprise.exists(), "the enterprise database was destroyed" + assert stray.read_text() == "keep me" + assert root.exists(), "the shared storage root itself was destroyed" + + +def test_adopts_a_legacy_database_this_identity_claimed(root): + """Upgraders keep using ``reflexio.db``; clearing must still reach it.""" + legacy = root / "reflexio.db" + _make_db(legacy, claimed_by=OUR_ORG) + + paths = _paths(cli._resolve_clear_all_targets()) + + assert legacy.resolve() in paths + + +def test_spares_a_legacy_database_another_identity_claimed(root): + legacy = root / "reflexio.db" + _make_db(legacy, claimed_by=OTHER_ORG) + + _clear() + + assert legacy.exists(), "a legacy database owned by another identity was destroyed" + + +def test_targets_an_unclaimed_legacy_database(root): + """No claim row means nobody has opened it since isolation landed. + + That is the pre-upgrade file this installation would adopt on its next + start, so it is ours to clear. + """ + legacy = root / "reflexio.db" + _make_db(legacy, claimed_by=None) + + paths = _paths(cli._resolve_clear_all_targets()) + + assert legacy.resolve() in paths + + +def test_resolution_creates_nothing(root): + """Resolution must be read-only: no mkdir, no claim, no empty database. + + ``resolve_sqlite_db_path`` upstream deliberately mutates (it mkdirs the root + and writes a claim row). Reusing it here would create a database in order to + delete it. + """ + before = {p.name for p in root.iterdir()} + + cli._resolve_clear_all_targets() + + assert {p.name for p in root.iterdir()} == before + + +def test_missing_root_resolves_without_creating_it(monkeypatch, tmp_path): + absent = tmp_path / "never-created" + monkeypatch.setenv("LOCAL_STORAGE_PATH", str(absent)) + monkeypatch.setenv("REFLEXIO_DEFAULT_ORG_ID", OUR_ORG) + monkeypatch.setattr(cli, "_REFLEXIO_CONFIG_PATH", tmp_path / "absent.json") + monkeypatch.setattr(cli, "_REFLEXIO_ENV_PATH", tmp_path / "absent.env") + + cli._resolve_clear_all_targets() + + assert not absent.exists() + + +def test_derived_filename_matches_the_canonical_resolver(): + """Anti-drift guard for the one thing this module duplicates. + + The plugin derives ``reflexio_.db`` itself rather than importing + ``_dataset_path``: that import costs ~1.7s through the storage package's + ``__init__``, and ``openclaw-smart-hook`` runs per session event. The + duplication is only safe while the two agree, so assert it against the + canonical implementation. Tests may pay the import cost the CLI cannot. + """ + # Importing `reflexio` runs its dotenv loader, which writes REFLEXIO_URL and + # friends into os.environ. Left alone that leaks into every later test in + # the run -- it silently broke test_reflexio_adapter's default-URL case, + # which passes in isolation and failed only in a full-suite run. + original_environ = dict(os.environ) + try: + from reflexio.server.services.storage.sqlite_storage._dataset_path import ( + LEGACY_DB_FILENAME, + derive_db_path, + ) + + assert cli._LEGACY_DB_FILENAME == LEGACY_DB_FILENAME + for org in ("self-host-org", "acme", "a.b-c_1"): + assert cli._derive_db_filename(org) == derive_db_path("/root", org).name + finally: + os.environ.clear() + os.environ.update(original_environ)