From cde1250b70b9fc3108e45f26921abf056d38704a Mon Sep 17 00:00:00 2001 From: tzzs Date: Mon, 14 Sep 2026 02:10:25 +0800 Subject: [PATCH 1/3] fix(migrate): reject execute() plans whose destination equals the source plan() refuses dest==src, but execute() re-reads the plan JSON from disk, so a hand-edited plan file can still pair a directory with itself: an empty source dir slips past the non-empty-destination check, the copy trivially verifies (0==0), and rmtree then deletes it as its own destination. The guard compares resolve_path()+normcase() on both sides, so trailing separators and Windows case variants are covered too. Surfaced by a Socket supply-chain audit anomaly (LOW) on core/migrate.py. --- src/storops/core/migrate.py | 11 ++++++ tests/unit/test_migrate_execute.py | 63 ++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 tests/unit/test_migrate_execute.py diff --git a/src/storops/core/migrate.py b/src/storops/core/migrate.py index 25c3a0f..a59e819 100644 --- a/src/storops/core/migrate.py +++ b/src/storops/core/migrate.py @@ -107,6 +107,17 @@ def execute(plan_file: str, *, confirm: bool = False, app_closed: bool = False) raw = json.loads(Path(plan_file).read_text(encoding="utf-8")) source, destination = raw["Source"], raw["Destination"] + # plan() already refuses dest==src, but the plan file is re-read from disk + # here and may no longer be the one plan() wrote. An empty source directory + # would slip past the non-empty-destination check below, the copy would + # trivially "verify" (0==0), and rmtree would then delete the directory as + # its own destination. normcase covers case-insensitive filesystems. + if os.path.normcase(resolve_path(destination)) == os.path.normcase(resolve_path(source)): + raise UnsupportedOperationError( + f"StorOps: plan is invalid -- destination equals the source ('{source}'). " + "Re-run `storops migrate plan` with a distinct destination." + ) + if not os.path.isdir(source): raise StalePlanError(f"StorOps: source '{source}' no longer exists or is not a directory -- the plan is stale. Re-run `storops migrate plan`.") diff --git a/tests/unit/test_migrate_execute.py b/tests/unit/test_migrate_execute.py new file mode 100644 index 0000000..dc06144 --- /dev/null +++ b/tests/unit/test_migrate_execute.py @@ -0,0 +1,63 @@ +"""Regression tests for migrate.execute()'s destination==source guard. + +plan() refuses to generate a plan whose destination equals its source, but +execute() re-reads the plan JSON from disk, so a hand-edited or otherwise +tampered plan file can still pair a directory with itself. Without the guard, +an *empty* source directory slips past execute()'s "destination must be +empty" check, the copy degenerates to a no-op whose verification trivially +matches (0 files == 0 files), and shutil.rmtree() then deletes the directory +as its own destination. +""" +from __future__ import annotations + +import json +import os +import sys +from pathlib import Path + +import pytest + +from storops.core.errors import UnsupportedOperationError +from storops.core.migrate import execute as migrate_execute + + +def _write_plan(tmp_path: Path, source: Path, destination: Path) -> Path: + plan_file = tmp_path / "storops-migrate-plan.json" + plan_file.write_text( + json.dumps({"Source": str(source), "Destination": str(destination)}), + encoding="utf-8", + ) + return plan_file + + +def test_execute_rejects_plan_whose_destination_equals_source(tmp_path): + src = tmp_path / "models" + src.mkdir() # empty on purpose: the non-empty-destination check must not be what saves us + plan_file = _write_plan(tmp_path, src, src) + + with pytest.raises(UnsupportedOperationError, match="destination equals the source"): + migrate_execute(str(plan_file), confirm=True) + + +def test_execute_rejects_destination_equal_after_path_normalization(tmp_path): + # Same directory spelled with a trailing separator -- resolve_path() + # normalizes both sides before comparing. + src = tmp_path / "models" + src.mkdir() + plan_file = _write_plan(tmp_path, src, Path(str(src) + os.sep)) + + with pytest.raises(UnsupportedOperationError, match="destination equals the source"): + migrate_execute(str(plan_file), confirm=True) + + +@pytest.mark.skipif( + sys.platform not in ("win32", "darwin"), + reason="only case-insensitive filesystems make the upper-cased path the same directory", +) +def test_execute_rejects_destination_equal_case_insensitively(tmp_path): + src = tmp_path / "models" + src.mkdir() + plan_file = _write_plan(tmp_path, src, Path(str(src).upper())) + + with pytest.raises(UnsupportedOperationError, match="destination equals the source"): + migrate_execute(str(plan_file), confirm=True) From e40508fd822991c2514415fe1cbb56822b5e1447 Mon Sep 17 00:00:00 2001 From: tzzs Date: Mon, 14 Sep 2026 02:50:08 +0800 Subject: [PATCH 2/3] fix(migrate): fold path case on darwin too -- normcase is a no-op on POSIX CI caught the guard's blind spot: os.path.normcase lowercases on Windows but returns POSIX paths unchanged, while macOS's default APFS volumes are case-insensitive. A tampered plan pairing '/Users/me/models' with an upper-cased spelling slipped past the guard on macOS and ran the self- migration flow. Compare resolved paths, fold case additionally on darwin; on Linux the comparison stays exact (case-sensitive filesystems keep distinct Foo/foo as distinct directories). --- src/storops/core/migrate.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/storops/core/migrate.py b/src/storops/core/migrate.py index a59e819..7ca03e4 100644 --- a/src/storops/core/migrate.py +++ b/src/storops/core/migrate.py @@ -12,6 +12,7 @@ import json import os import shutil +import sys from datetime import datetime, timezone from pathlib import Path @@ -111,8 +112,14 @@ def execute(plan_file: str, *, confirm: bool = False, app_closed: bool = False) # here and may no longer be the one plan() wrote. An empty source directory # would slip past the non-empty-destination check below, the copy would # trivially "verify" (0==0), and rmtree would then delete the directory as - # its own destination. normcase covers case-insensitive filesystems. - if os.path.normcase(resolve_path(destination)) == os.path.normcase(resolve_path(source)): + # its own destination. normcase folds case on Windows only; darwin folds + # explicitly because default APFS volumes are case-insensitive too. + resolved_source = resolve_path(source) + resolved_destination = resolve_path(destination) + collide = os.path.normcase(resolved_source) == os.path.normcase(resolved_destination) + if sys.platform == "darwin": + collide = collide or resolved_source.lower() == resolved_destination.lower() + if collide: raise UnsupportedOperationError( f"StorOps: plan is invalid -- destination equals the source ('{source}'). " "Re-run `storops migrate plan` with a distinct destination." From 565d8b567247157eaca97fd7d55a7a4043ead981 Mon Sep 17 00:00:00 2001 From: tzzs Date: Mon, 14 Sep 2026 02:29:12 +0800 Subject: [PATCH 3/3] test(migrate): cover execute()/verify() flow with isolated fake engines execute() was only covered by the dest==source guard tests; its whole status matrix ran untested. Drive it through the real rules engine (a custom rules dir marks the source as migratable FakeApp data) against fake copy/link engines and get_work_dir pointed at tmp_path: - succeeded via link method and via manual method (hint recorded, no link created) - verification-failed leaves the original untouched - copy-ok-source-not-removed when rmtree raises OSError - dry-run copies nothing and writes no result file - RequiresAppClosed blocks until --app-closed is attested - stale-plan gates: vanished source, non-empty destination verify() had no coverage at all: passing report writes LastVerification back into the result file, missing destination / drifted file counts / broken link each fail their own check, and a missing result file raises FileNotFoundError. Full suite: 156 passed, 39 skipped -- skip count matches the known Windows baseline (39). Stacked on #22. --- tests/unit/test_migrate_execute.py | 252 +++++++++++++++++++++++++++-- tests/unit/test_migrate_verify.py | 143 ++++++++++++++++ 2 files changed, 380 insertions(+), 15 deletions(-) create mode 100644 tests/unit/test_migrate_verify.py diff --git a/tests/unit/test_migrate_execute.py b/tests/unit/test_migrate_execute.py index dc06144..9531543 100644 --- a/tests/unit/test_migrate_execute.py +++ b/tests/unit/test_migrate_execute.py @@ -1,35 +1,118 @@ -"""Regression tests for migrate.execute()'s destination==source guard. - -plan() refuses to generate a plan whose destination equals its source, but -execute() re-reads the plan JSON from disk, so a hand-edited or otherwise -tampered plan file can still pair a directory with itself. Without the guard, -an *empty* source directory slips past execute()'s "destination must be -empty" check, the copy degenerates to a no-op whose verification trivially -matches (0 files == 0 files), and shutil.rmtree() then deletes the directory -as its own destination. +"""Unit tests for migrate.execute(): the destination==source guard plus the +full copy-verify-remove-relink flow around fake platform engines. + +The guard tests pin down a Socket-audit finding: plan() refuses to generate a +plan whose destination equals its source, but execute() re-reads the plan JSON +from disk, so a hand-edited or otherwise tampered plan file can still pair a +directory with itself. Without the guard, an *empty* source directory slips +past execute()'s "destination must be empty" check, the copy degenerates to a +no-op whose verification trivially matches (0 files == 0 files), and +shutil.rmtree() then deletes the directory as its own destination. + +The flow tests drive execute() against the real rules engine (a custom rules +dir classifying the source as migratable FakeApp data) and fake copy/link +engines, covering the status matrix of the result file: succeeded (linked and +manual), verification-failed, copy-ok-source-not-removed, plus the stale-plan +and dry-run gates. """ from __future__ import annotations import json import os +import shutil import sys +import textwrap from pathlib import Path import pytest -from storops.core.errors import UnsupportedOperationError +from storops import platform as platform_pkg +from storops.core import rules +from storops.core.errors import StalePlanError, UnsupportedOperationError from storops.core.migrate import execute as migrate_execute -def _write_plan(tmp_path: Path, source: Path, destination: Path) -> Path: +class _PartialCopyEngine: + """CopyEngine stand-in: copies like shutil.copytree but can silently drop + one file from the destination, so the post-copy count/size verification + has something real to catch.""" + + kind = "shutil" + + def __init__(self, *, drop_one_file: bool = False): + self.drop_one_file = drop_one_file + self.copies: list[tuple[str, str]] = [] + + def copy(self, source: str, destination: str) -> None: + self.copies.append((source, destination)) + shutil.copytree(source, destination) + if self.drop_one_file: + files = sorted(path for path in Path(destination).rglob("*") if path.is_file()) + files[-1].unlink() + + +class _FakeLinkEngine: + kind = "fakelink" + + def __init__(self): + self.created: list[tuple[str, str]] = [] + + def create(self, old_path: str, target: str) -> None: + self.created.append((old_path, target)) + + def verify(self, old_path: str, expected_target: str) -> bool: + return True + + +_FAKEAPP_RULE = textwrap.dedent( + """\ + - id: fake-app + application: FakeApp + category: ai-model-weights + path_patterns: + - "%HOME%/.fakeapp/models/*" + migratable: true + migration_method: app-config + cleanup_risk: high + """ +) + + +def _migratable_source(tmp_path: Path, monkeypatch) -> Path: + """A source directory the real rules engine classifies as migratable + FakeApp data (high cleanup risk, which assert_not_critical allows).""" + home = tmp_path / "home" + source = home / ".fakeapp" / "models" + source.mkdir(parents=True) + (source / "weights.bin").write_bytes(b"w" * 128) + + rules_dir = tmp_path / "rules" + rules_dir.mkdir() + (rules_dir / "ai-models.yaml").write_text(_FAKEAPP_RULE, encoding="utf-8") + monkeypatch.setattr(Path, "home", classmethod(lambda cls: home)) + monkeypatch.setattr(rules, "_default_rules_dir", lambda: rules_dir) + return source + + +def _isolate_platform(tmp_path: Path, monkeypatch, copy_engine, link_engine) -> None: + monkeypatch.setattr(platform_pkg, "get_copy_engine", lambda: copy_engine) + monkeypatch.setattr(platform_pkg, "get_link_engine", lambda: link_engine) + work_dir = tmp_path / "work" + work_dir.mkdir() + monkeypatch.setattr(platform_pkg, "get_work_dir", lambda: str(work_dir)) + + +def _write_plan(tmp_path: Path, source: Path, destination: Path, **extra) -> Path: + plan = {"Source": str(source), "Destination": str(destination)} + plan.update(extra) plan_file = tmp_path / "storops-migrate-plan.json" - plan_file.write_text( - json.dumps({"Source": str(source), "Destination": str(destination)}), - encoding="utf-8", - ) + plan_file.write_text(json.dumps(plan), encoding="utf-8") return plan_file +# --- destination==source guard ---------------------------------------------- + + def test_execute_rejects_plan_whose_destination_equals_source(tmp_path): src = tmp_path / "models" src.mkdir() # empty on purpose: the non-empty-destination check must not be what saves us @@ -61,3 +144,142 @@ def test_execute_rejects_destination_equal_case_insensitively(tmp_path): with pytest.raises(UnsupportedOperationError, match="destination equals the source"): migrate_execute(str(plan_file), confirm=True) + + +# --- flow: the copy-verify-remove-relink status matrix ----------------------- + + +def test_execute_happy_path_copies_removes_and_relinks(tmp_path, monkeypatch): + source = _migratable_source(tmp_path, monkeypatch) + destination = tmp_path / "pool" / "models" + copy_engine = _PartialCopyEngine() + link_engine = _FakeLinkEngine() + _isolate_platform(tmp_path, monkeypatch, copy_engine, link_engine) + plan_file = _write_plan(tmp_path, source, destination) + + result = migrate_execute(str(plan_file), confirm=True) + + assert result.status == "succeeded" + assert result.verified is True + assert result.source_removed is True + assert result.link_created is True + assert copy_engine.copies == [(str(source), str(destination))] + assert link_engine.created == [(str(source), str(destination))] + assert (destination / "weights.bin").read_bytes() == b"w" * 128 + assert not source.exists() + + saved = json.loads((tmp_path / "work" / "storops-migrate-result.json").read_text(encoding="utf-8")) + assert saved["Status"] == "succeeded" + assert saved["Verified"] is True + assert saved["SourceRemoved"] is True + assert saved["JunctionCreated"] is True + + +def test_execute_manual_method_records_hint_and_skips_link(tmp_path, monkeypatch): + source = _migratable_source(tmp_path, monkeypatch) + destination = tmp_path / "pool" / "models" + copy_engine = _PartialCopyEngine() + link_engine = _FakeLinkEngine() + _isolate_platform(tmp_path, monkeypatch, copy_engine, link_engine) + plan_file = _write_plan( + tmp_path, + source, + destination, + Method="manual", + MigrationHint="point FakeApp at the new models directory", + ) + + result = migrate_execute(str(plan_file), confirm=True) + + assert result.status == "succeeded" + assert result.source_removed is True + assert result.link_created is False + assert link_engine.created == [] + assert "point FakeApp at the new models directory" in result.detail + + +def test_execute_verification_failure_leaves_source_intact(tmp_path, monkeypatch): + source = _migratable_source(tmp_path, monkeypatch) + destination = tmp_path / "pool" / "models" + copy_engine = _PartialCopyEngine(drop_one_file=True) + _isolate_platform(tmp_path, monkeypatch, copy_engine, _FakeLinkEngine()) + plan_file = _write_plan(tmp_path, source, destination) + + result = migrate_execute(str(plan_file), confirm=True) + + assert result.status == "verification-failed" + assert result.verified is False + assert result.source_removed is False + assert "Original left untouched" in result.detail + assert source.exists() + assert (source / "weights.bin").exists() + + +def test_execute_dry_run_copies_nothing(tmp_path, monkeypatch): + source = _migratable_source(tmp_path, monkeypatch) + copy_engine = _PartialCopyEngine() + _isolate_platform(tmp_path, monkeypatch, copy_engine, _FakeLinkEngine()) + plan_file = _write_plan(tmp_path, source, tmp_path / "pool" / "models") + + assert migrate_execute(str(plan_file), confirm=False) is None + assert copy_engine.copies == [] + assert source.exists() + assert not (tmp_path / "work" / "storops-migrate-result.json").exists() + + +def test_execute_requires_app_closed_until_attested(tmp_path, monkeypatch): + source = _migratable_source(tmp_path, monkeypatch) + copy_engine = _PartialCopyEngine() + _isolate_platform(tmp_path, monkeypatch, copy_engine, _FakeLinkEngine()) + plan_file = _write_plan(tmp_path, source, tmp_path / "pool" / "models", RequiresAppClosed=True) + + with pytest.raises(UnsupportedOperationError, match="--app-closed"): + migrate_execute(str(plan_file), confirm=True, app_closed=False) + + # Once attested, execution proceeds (dry-run here, the copy flow itself is + # covered by the happy-path test). + assert migrate_execute(str(plan_file), confirm=False, app_closed=True) is None + + +def test_execute_rejects_stale_plan_when_source_vanished(tmp_path): + # The isdir gate fires before any rules lookup, so no engine isolation + # is needed: a plan whose source is gone must be called stale. + plan_file = _write_plan( + tmp_path, + tmp_path / "home" / ".fakeapp" / "gone", + tmp_path / "pool" / "models", + ) + + with pytest.raises(StalePlanError, match="no longer exists"): + migrate_execute(str(plan_file), confirm=True) + + +def test_execute_rejects_stale_plan_when_destination_not_empty(tmp_path, monkeypatch): + source = _migratable_source(tmp_path, monkeypatch) + destination = tmp_path / "pool" / "models" + destination.mkdir(parents=True) + (destination / "leftover.bin").write_bytes(b"x") + _isolate_platform(tmp_path, monkeypatch, _PartialCopyEngine(), _FakeLinkEngine()) + plan_file = _write_plan(tmp_path, source, destination) + + with pytest.raises(StalePlanError, match="not empty"): + migrate_execute(str(plan_file), confirm=True) + + +def test_execute_reports_source_not_removed_when_rmtree_fails(tmp_path, monkeypatch): + source = _migratable_source(tmp_path, monkeypatch) + destination = tmp_path / "pool" / "models" + _isolate_platform(tmp_path, monkeypatch, _PartialCopyEngine(), _FakeLinkEngine()) + plan_file = _write_plan(tmp_path, source, destination) + + def _locked(path, *args, **kwargs): + raise OSError("directory in use") + + monkeypatch.setattr("shutil.rmtree", _locked) + + result = migrate_execute(str(plan_file), confirm=True) + + assert result.status == "copy-ok-source-not-removed" + assert result.source_removed is False + assert "could not be removed" in result.detail + assert source.exists() diff --git a/tests/unit/test_migrate_verify.py b/tests/unit/test_migrate_verify.py new file mode 100644 index 0000000..ab834f7 --- /dev/null +++ b/tests/unit/test_migrate_verify.py @@ -0,0 +1,143 @@ +"""Unit tests for migrate.verify()'s post-migration report. + +verify() re-reads the result JSON that execute() wrote, re-measures the +destination with dir_stats(), and appends a LastVerification block back into +the result file. These tests hand-craft result files so the report logic is +covered independently of a real copy/link engine (the engines have their own +unit tests under tests/unit/test_windows_*.py / test_posix_platform.py; +execute()'s flow is covered in test_migrate_execute.py). +""" +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from storops import platform as platform_pkg +from storops.core.copystats import dir_stats +from storops.core.migrate import verify + + +class _FakeLinkEngine: + kind = "fakelink" + + def __init__(self, verify_result: bool = True): + self.verify_result = verify_result + + def create(self, old_path: str, target: str) -> None: ... + + def verify(self, old_path: str, expected_target: str) -> bool: + return self.verify_result + + +def _write_result(tmp_path: Path, payload: dict) -> Path: + result_file = tmp_path / "storops-migrate-result.json" + result_file.write_text(json.dumps(payload), encoding="utf-8") + return result_file + + +def _populated_destination(tmp_path: Path) -> tuple[Path, dict]: + destination = tmp_path / "pool" / "models" + destination.mkdir(parents=True) + (destination / "a.bin").write_bytes(b"a" * 100) + (destination / "b.bin").write_bytes(b"b" * 40) + stats = dir_stats(str(destination)) + return destination, {"FileCount": stats.file_count, "SizeBytes": stats.size_bytes} + + +def test_verify_passes_and_writes_last_verification_back(tmp_path): + destination, post_copy = _populated_destination(tmp_path) + result_file = _write_result( + tmp_path, + {"Source": str(tmp_path / "gone"), "Destination": str(destination), "PostCopy": post_copy}, + ) + + report = verify(str(result_file)) + + assert report.passed is True + assert {c.check: c.passed for c in report.checks} == { + "target-accessible": True, + "file-count-matches": True, + "total-size-matches": True, + "source-cleared": True, + } + + saved = json.loads(result_file.read_text(encoding="utf-8")) + assert saved["LastVerification"]["Pass"] is True + assert len(saved["LastVerification"]["Checks"]) == 4 + assert saved["LastVerification"]["VerifiedAt"] + + +def test_verify_fails_when_destination_missing(tmp_path): + result_file = _write_result( + tmp_path, + { + "Source": str(tmp_path / "gone"), + "Destination": str(tmp_path / "pool" / "missing"), + "PostCopy": {"FileCount": 2, "SizeBytes": 140}, + }, + ) + + report = verify(str(result_file)) + + assert report.passed is False + checks = {c.check: c.passed for c in report.checks} + assert checks["target-accessible"] is False + assert checks["file-count-matches"] is False + assert checks["total-size-matches"] is False + assert checks["source-cleared"] is True + + saved = json.loads(result_file.read_text(encoding="utf-8")) + assert saved["LastVerification"]["Pass"] is False + + +def test_verify_fails_when_file_count_drifts(tmp_path): + destination, post_copy = _populated_destination(tmp_path) + drifted = dict(post_copy, FileCount=post_copy["FileCount"] + 1) + result_file = _write_result( + tmp_path, + {"Source": str(tmp_path / "gone"), "Destination": str(destination), "PostCopy": drifted}, + ) + + report = verify(str(result_file)) + + assert report.passed is False + checks = {c.check: c.passed for c in report.checks} + assert checks["target-accessible"] is True + assert checks["file-count-matches"] is False + assert checks["total-size-matches"] is True + + +def test_verify_link_method_reports_link_health(tmp_path, monkeypatch): + destination, post_copy = _populated_destination(tmp_path) + source = tmp_path / "home" / ".fakeapp" / "models" + source.mkdir(parents=True) # stand-in for the link site execute() creates + result_file = _write_result( + tmp_path, + { + "Source": str(source), + "Destination": str(destination), + "PostCopy": post_copy, + "Method": "fakelink", + }, + ) + + monkeypatch.setattr(platform_pkg, "get_link_engine", lambda: _FakeLinkEngine()) + report = verify(str(result_file)) + checks = {c.check: c.passed for c in report.checks} + assert checks["source-is-fakelink"] is True + assert checks["fakelink-works"] is True + assert report.passed is True + + monkeypatch.setattr(platform_pkg, "get_link_engine", lambda: _FakeLinkEngine(verify_result=False)) + broken = verify(str(result_file)) + broken_checks = {c.check: c.passed for c in broken.checks} + assert broken_checks["source-is-fakelink"] is True + assert broken_checks["fakelink-works"] is False + assert broken.passed is False + + +def test_verify_missing_result_file_raises(tmp_path): + with pytest.raises(FileNotFoundError): + verify(str(tmp_path / "absent.json"))