From 9f104795f4c78e4efc4bfabf74ff3dcf51d3aa8d Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Thu, 28 May 2026 04:34:17 +0000 Subject: [PATCH 1/9] =?UTF-8?q?chore:=20PLAN04-migration=20Draft=20PR=20?= =?UTF-8?q?=E4=BD=9C=E6=88=90?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From 32b648341654f3c4e788b7c1cc49590f1a683b8c Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Thu, 28 May 2026 06:30:11 +0000 Subject: [PATCH 2/9] =?UTF-8?q?feat(plugin):=20=E6=97=A2=E5=AD=98=20plugin?= =?UTF-8?q?s/=20=E3=82=B3=E3=83=94=E3=83=BC=E3=82=A4=E3=83=B3=E3=82=B9?= =?UTF-8?q?=E3=83=88=E3=83=BC=E3=83=AB=E3=82=92=20repos/=20=E6=B0=B8?= =?UTF-8?q?=E7=B6=9A=E3=82=AF=E3=83=AD=E3=83=BC=E3=83=B3=E3=81=B8=E7=A7=BB?= =?UTF-8?q?=E8=A1=8C?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PLAN04 PR2。PR1 (#29) で repos/ 永続クローン方式に切り替えたが、PR1 以前に plugins// へファイルコピーされた既存インストールは移行されないため、その 移行ロジックを追加する。 - migrator.py (新規): - needs_migration / _is_legacy_plugin: legacy plugins/ インストールの検出 (linked は --link 専用として除外) - _dirs_differ: コピーとクローンの差分検出 (内容変更・追加ファイルを保守的に差分扱い) - migrate: 未クローン repo の永続クローン作成、InstalledPlugin.path の repos/ 書き換え、 差分なしは plugins/ 削除・差分ありは .bak 保全、sync_projects 再実行、 --link/.bak/skip が無ければ plugins/ を .gitkeep のみに正規化 - plugin migrate サブコマンド (cli.py / commands/plugin.py) - install/update 初回実行時に _auto_migrate で自動移行 Co-Authored-By: Claude Opus 4.7 --- lib/devbase/cli.py | 3 + lib/devbase/commands/plugin.py | 32 +++ lib/devbase/plugin/installer.py | 25 +++ lib/devbase/plugin/migrator.py | 240 ++++++++++++++++++++ lib/devbase/plugin/updater.py | 3 + tests/plugin/test_migrator.py | 376 ++++++++++++++++++++++++++++++++ 6 files changed, 679 insertions(+) create mode 100644 lib/devbase/plugin/migrator.py create mode 100644 tests/plugin/test_migrator.py diff --git a/lib/devbase/cli.py b/lib/devbase/cli.py index 9cd1ce1..6512c2e 100644 --- a/lib/devbase/cli.py +++ b/lib/devbase/cli.py @@ -234,6 +234,9 @@ def _add_plugin_parser(subparsers): pl_sub.add_parser('sync', help='Resync project symlinks') + pl_sub.add_parser('migrate', + help='Migrate legacy plugins/ installs to repos/ clones') + # Plugin repo sub-subcommands pl_repo = pl_sub.add_parser('repo', help='Manage plugin repositories') pl_repo_sub = pl_repo.add_subparsers(dest='repo_command') diff --git a/lib/devbase/commands/plugin.py b/lib/devbase/commands/plugin.py index b20d31d..f6acf61 100644 --- a/lib/devbase/commands/plugin.py +++ b/lib/devbase/commands/plugin.py @@ -9,6 +9,7 @@ from devbase.plugin.updater import update_plugin from devbase.plugin.info import show_plugin_info, show_available_plugins from devbase.plugin.syncer import sync_projects +from devbase.plugin.migrator import migrate from devbase.plugin.repo_manager import ( add_repository, remove_repository, @@ -34,6 +35,7 @@ def cmd_plugin(devbase_root: Path, args) -> int: 'update': lambda: cmd_plugin_update(devbase_root, getattr(args, 'name', None)), 'info': lambda: cmd_plugin_info(devbase_root, getattr(args, 'name', '')), 'sync': lambda: cmd_sync(devbase_root), + 'migrate': lambda: cmd_plugin_migrate(devbase_root), 'repo': lambda: cmd_repo(devbase_root, args), } @@ -138,6 +140,36 @@ def cmd_sync(devbase_root: Path) -> int: return 0 +def cmd_plugin_migrate(devbase_root: Path) -> int: + """Migrate legacy plugins/ copy installs to repos/ persistent clones""" + registry = PluginRegistry(devbase_root) + try: + result = migrate(registry) + except DevbaseError as e: + logger.error("%s", e) + return 1 + + if not (result.migrated or result.preserved or result.skipped): + logger.info("No legacy plugins/ installs to migrate.") + return 0 + + if result.migrated: + logger.info("Migrated %d plugin(s) to repos/: %s", + len(result.migrated), ", ".join(result.migrated)) + if result.preserved: + logger.warning( + "Preserved %d plugin(s) with local changes as plugins/.bak " + "(reconcile manually): %s", + len(result.preserved), ", ".join(result.preserved)) + if result.skipped: + logger.warning("Could not migrate %d plugin(s): %s", + len(result.skipped), ", ".join(result.skipped)) + for err in result.errors: + logger.warning(" %s", err) + return 1 + return 0 + + def cmd_repo(devbase_root: Path, args) -> int: """Dispatch repo subcommands""" registry = PluginRegistry(devbase_root) diff --git a/lib/devbase/plugin/installer.py b/lib/devbase/plugin/installer.py index 747c548..5cb157a 100644 --- a/lib/devbase/plugin/installer.py +++ b/lib/devbase/plugin/installer.py @@ -81,6 +81,29 @@ def resolve_repo_url(repo: str) -> str: return f"https://github.com/{repo}.git" +def _auto_migrate(registry: PluginRegistry) -> None: + """Migrate any legacy plugins/ copy installs to repos/ before proceeding. + + Triggered on the first install/update after upgrading to repos/-based + plugin management so users do not have to run `devbase plugin migrate` + manually. No-op when nothing legacy remains. + """ + from .migrator import migrate, needs_migration + if not needs_migration(registry): + return + logger.info("Legacy plugins/ installs detected — migrating to repos/...") + result = migrate(registry) + if result.migrated: + logger.info(" Migrated: %s", ", ".join(result.migrated)) + if result.preserved: + logger.warning( + " Preserved with local changes (plugins/.bak): %s", + ", ".join(result.preserved), + ) + if result.skipped: + logger.warning(" Could not migrate: %s", ", ".join(result.skipped)) + + def install_plugin( registry: PluginRegistry, source_str: str, @@ -91,6 +114,8 @@ def install_plugin( Raises PluginError on failure. """ + _auto_migrate(registry) + source = PluginSource.parse(source_str, link=link) plugins_dir = registry.get_plugins_dir() diff --git a/lib/devbase/plugin/migrator.py b/lib/devbase/plugin/migrator.py new file mode 100644 index 0000000..b9df50f --- /dev/null +++ b/lib/devbase/plugin/migrator.py @@ -0,0 +1,240 @@ +"""Plugin migrator - migrates legacy plugins/ copy installs to repos/ clones""" + +import shutil +from dataclasses import dataclass, field +from pathlib import Path + +from devbase.errors import PluginError +from devbase.log import get_logger + +from .models import AvailablePlugin, InstalledPlugin, RegisteredRepository +from .registry import PluginRegistry + +logger = get_logger("devbase.plugin.migrator") + + +@dataclass +class MigrationResult: + """Outcome of a plugins/ -> repos/ migration run.""" + migrated: list[str] = field(default_factory=list) # cleanly moved + copy deleted + preserved: list[str] = field(default_factory=list) # copy differed -> kept as .bak + skipped: list[str] = field(default_factory=list) # could not migrate + errors: list[str] = field(default_factory=list) + plugins_dir_cleaned: bool = False # plugins/ emptied to .gitkeep + + +def _is_legacy_plugin(plugin: InstalledPlugin) -> bool: + """True if a plugin still uses the pre-PLAN04 plugins/ copy format. + + --link installs also live under plugins/ but are intentional and must not + be migrated, so they are excluded. + """ + if plugin.linked: + return False + parts = Path(plugin.path).parts + return len(parts) >= 1 and parts[0] == 'plugins' + + +def needs_migration(registry: PluginRegistry) -> bool: + """True if any installed plugin still uses the legacy plugins/ copy format.""" + return any(_is_legacy_plugin(p) for p in registry.list_installed()) + + +def _dirs_differ(copy_dir: Path, repo_dir: Path) -> bool: + """True if the legacy copy differs in any way from the repos/ clone dir. + + Compares the set of regular files and their byte content. Any divergence + (changed content, a user-added file, or an upstream-added file) is treated + as a difference so the copy is preserved rather than deleted — migration + must never silently discard data. + """ + def _files(base: Path) -> set[Path]: + return { + p.relative_to(base) + for p in base.rglob('*') + if p.is_file() and not p.is_symlink() + } + + copy_files = _files(copy_dir) + repo_files = _files(repo_dir) + if copy_files != repo_files: + return True + + for rel in copy_files: + fa, fb = copy_dir / rel, repo_dir / rel + if fa.stat().st_size != fb.stat().st_size: + return True + if fa.read_bytes() != fb.read_bytes(): + return True + return False + + +def _ensure_repo_cloned( + registry: PluginRegistry, repo: RegisteredRepository, +) -> tuple[Path, RegisteredRepository]: + """Return the repos/ clone dir for a repo, cloning it if necessary. + + If the repo was registered before persistent-clone support (no local_path) + or the clone dir is missing, perform a full clone and persist local_path + + a refreshed plugin list to plugins.yml. + """ + from .installer import git_clone, parse_registry_yml + from .repo_manager import _url_to_repos_dirname + + if repo.local_path: + clone_dir = registry.devbase_root / repo.local_path + if clone_dir.is_dir(): + return clone_dir, repo + + dir_name = _url_to_repos_dirname(repo.url) + repos_dir = registry.get_repos_dir() + repos_dir.mkdir(exist_ok=True) + clone_dir = repos_dir / dir_name + + if not clone_dir.is_dir(): + git_clone(repo.url, clone_dir, shallow=False) + + reg_info = parse_registry_yml(clone_dir) + if not reg_info: + raise PluginError( + f"No registry.yml found in cloned repository '{repo.name}'." + ) + + local_path = f"repos/{dir_name}" + updated = RegisteredRepository( + name=repo.name, url=repo.url, added_at=repo.added_at, + local_path=local_path, + plugins=[ + AvailablePlugin(name=e.name, description=e.description, path=e.path) + for e in reg_info.plugins + ], + ) + registry.add_repository(updated) + logger.info("Repository '%s' cloned to %s", repo.name, local_path) + return clone_dir, updated + + +def _cleanup_plugins_dir(registry: PluginRegistry) -> bool: + """Normalize plugins/ to just .gitkeep once it holds no live copy installs. + + Conservatively kept untouched when anything still depends on it: + --link installs, skipped legacy copies still referenced by plugins.yml, or + preserved .bak directories awaiting manual reconciliation. Returns + True only when plugins/ was reduced to .gitkeep. + """ + plugins_dir = registry.get_plugins_dir() + if not plugins_dir.is_dir(): + return False + + if any(p.linked for p in registry.list_installed()): + return False + + # A skipped legacy install still points into plugins/ — keep its files. + if needs_migration(registry): + return False + + entries = [e for e in plugins_dir.iterdir() if e.name != '.gitkeep'] + bak_dirs = [e for e in entries if e.name.endswith('.bak')] + if bak_dirs: + logger.info( + "plugins/ retained: %d preserved .bak dir(s) await manual reconciliation", + len(bak_dirs), + ) + return False + + gitkeep = plugins_dir / '.gitkeep' + if not gitkeep.exists(): + gitkeep.touch() + return True + + +def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResult: + """Migrate legacy plugins/ copy installs to repos/ clones. + + For each legacy plugin: ensure its source repo is cloned to repos/, rewrite + InstalledPlugin.path to the repos/ location, then delete the old copy when + byte-identical or preserve it as .bak when it diverged. Finally + re-sync project symlinks and empty plugins/ to .gitkeep when safe. + """ + from .installer import parse_registry_yml + from .syncer import load_plugin_info, sync_projects + + result = MigrationResult() + legacy = [p for p in registry.list_installed() if _is_legacy_plugin(p)] + if not legacy: + return result + + for plugin in legacy: + try: + repo = registry.get_repository_by_url(plugin.source) + if not repo: + result.skipped.append(plugin.name) + result.errors.append( + f"{plugin.name}: source repository not registered " + f"({plugin.source or 'no source URL'})" + ) + continue + + clone_dir, repo = _ensure_repo_cloned(registry, repo) + + reg_info = parse_registry_yml(clone_dir) + entry = None + if reg_info: + entry = next( + (e for e in reg_info.plugins if e.name == plugin.name), None, + ) + if not entry: + result.skipped.append(plugin.name) + result.errors.append( + f"{plugin.name}: not found in registry.yml of '{repo.name}'" + ) + continue + + repo_plugin_dir = clone_dir / entry.path.rstrip('/') + if not repo_plugin_dir.is_dir(): + result.skipped.append(plugin.name) + result.errors.append( + f"{plugin.name}: plugin dir missing in clone: {repo_plugin_dir}" + ) + continue + + rel_path = str(repo_plugin_dir.relative_to(registry.devbase_root)) + info = load_plugin_info(repo_plugin_dir) + version = info.version if info else plugin.version + + registry.add(InstalledPlugin( + name=plugin.name, + version=version, + source=plugin.source, + installed_at=plugin.installed_at, + path=rel_path, + linked=False, + )) + + old_dir = registry.devbase_root / plugin.path + if old_dir.is_dir() and not old_dir.is_symlink(): + if _dirs_differ(old_dir, repo_plugin_dir): + bak = old_dir.with_name(old_dir.name + '.bak') + if bak.exists(): + shutil.rmtree(bak) + old_dir.rename(bak) + result.preserved.append(plugin.name) + logger.warning( + "Plugin '%s' had local changes — preserved at %s " + "(reconcile manually, then remove)", + plugin.name, bak, + ) + else: + shutil.rmtree(old_dir) + result.migrated.append(plugin.name) + else: + result.migrated.append(plugin.name) + except Exception as e: + result.skipped.append(plugin.name) + result.errors.append(f"{plugin.name}: {e}") + + if run_sync: + sync_projects(registry) + + result.plugins_dir_cleaned = _cleanup_plugins_dir(registry) + return result diff --git a/lib/devbase/plugin/updater.py b/lib/devbase/plugin/updater.py index 4fdd163..d66d18a 100644 --- a/lib/devbase/plugin/updater.py +++ b/lib/devbase/plugin/updater.py @@ -189,6 +189,9 @@ def update_plugin(registry: PluginRegistry, name: Optional[str] = None) -> None: Raises PluginError on failure. """ + from .installer import _auto_migrate + _auto_migrate(registry) + installed = registry.list_installed() if not installed: logger.info("No plugins installed") diff --git a/tests/plugin/test_migrator.py b/tests/plugin/test_migrator.py new file mode 100644 index 0000000..fc692c4 --- /dev/null +++ b/tests/plugin/test_migrator.py @@ -0,0 +1,376 @@ +"""Tests for PLAN04 PR2: legacy plugins/ -> repos/ migration""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import patch + +import yaml +import pytest + +from devbase.plugin.models import ( + AvailablePlugin, + InstalledPlugin, + RegisteredRepository, +) +from devbase.plugin.registry import PluginRegistry +from devbase.plugin.migrator import ( + _dirs_differ, + _is_legacy_plugin, + migrate, + needs_migration, +) + +URL = "https://github.com/testorg/testrepo.git" +DIRNAME = "github.com--testorg--testrepo" + + +def _make_repo_clone(devbase_root: Path, plugins: list[dict]) -> Path: + """Create repos// with .git, registry.yml and plugin dirs.""" + repo_dir = devbase_root / "repos" / DIRNAME + repo_dir.mkdir(parents=True, exist_ok=True) + (repo_dir / ".git").mkdir(exist_ok=True) + + entries = [] + for p in plugins: + pdir = repo_dir / p["path"] + pdir.mkdir(parents=True, exist_ok=True) + (pdir / "plugin.yml").write_text( + yaml.dump({"name": p["name"], "version": p.get("version", "1.0.0")}) + ) + for proj in p.get("projects", []): + proj_dir = pdir / "projects" / proj + proj_dir.mkdir(parents=True, exist_ok=True) + (proj_dir / "compose.yml").write_text("services: {}\n") + entries.append({"name": p["name"], "path": p["path"], "description": ""}) + + (repo_dir / "registry.yml").write_text( + yaml.dump({"name": "testrepo", "plugins": entries}) + ) + return repo_dir + + +def _register_repo(registry: PluginRegistry, plugins: list[dict], + local_path: str | None = f"repos/{DIRNAME}") -> None: + registry.add_repository(RegisteredRepository( + name="testrepo", url=URL, added_at=registry.now_iso(), + local_path=local_path or "", + plugins=[ + AvailablePlugin(name=p["name"], description="", path=p["path"]) + for p in plugins + ], + )) + + +def _make_legacy_copy(devbase_root: Path, name: str, plugin: dict, + extra: dict[str, str] | None = None) -> Path: + """Create plugins// mirroring the repo plugin dir (optionally diverged).""" + pdir = devbase_root / "plugins" / name + pdir.mkdir(parents=True, exist_ok=True) + (pdir / "plugin.yml").write_text( + yaml.dump({"name": plugin["name"], "version": plugin.get("version", "1.0.0")}) + ) + for proj in plugin.get("projects", []): + proj_dir = pdir / "projects" / proj + proj_dir.mkdir(parents=True, exist_ok=True) + (proj_dir / "compose.yml").write_text("services: {}\n") + for rel, content in (extra or {}).items(): + f = pdir / rel + f.parent.mkdir(parents=True, exist_ok=True) + f.write_text(content) + return pdir + + +# ── Fixtures ──────────────────────────────────────────────────── + + +@pytest.fixture +def devbase_root(tmp_path): + (tmp_path / "projects").mkdir() + return tmp_path + + +@pytest.fixture +def registry(devbase_root): + return PluginRegistry(devbase_root) + + +def _installed(name: str, path: str, linked: bool = False) -> InstalledPlugin: + return InstalledPlugin( + name=name, + version="1.0.0", + source="https://github.com/testorg/testrepo.git", + installed_at="2026-01-01T00:00:00+00:00", + path=path, + linked=linked, + ) + + +# ── detection ─────────────────────────────────────────────────── + + +class TestIsLegacyPlugin: + def test_copy_install_under_plugins_is_legacy(self): + assert _is_legacy_plugin(_installed("adminer", "plugins/adminer")) is True + + def test_repos_based_is_not_legacy(self): + plugin = _installed( + "adminer", "repos/github.com--testorg--testrepo/adminer", + ) + assert _is_legacy_plugin(plugin) is False + + def test_linked_under_plugins_is_not_legacy(self): + plugin = _installed("local", "plugins/local", linked=True) + assert _is_legacy_plugin(plugin) is False + + +class TestDirsDiffer: + def _write(self, base: Path, rel: str, content: str) -> None: + p = base / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(content) + + def test_identical_dirs_do_not_differ(self, tmp_path): + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: x\n") + self._write(a, "projects/p/compose.yml", "services: {}\n") + self._write(b, "plugin.yml", "name: x\n") + self._write(b, "projects/p/compose.yml", "services: {}\n") + assert _dirs_differ(a, b) is False + + def test_changed_file_content_differs(self, tmp_path): + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: x\nversion: 9\n") + self._write(b, "plugin.yml", "name: x\n") + assert _dirs_differ(a, b) is True + + def test_extra_file_in_a_differs(self, tmp_path): + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: x\n") + self._write(a, "projects/p/.env", "SECRET=1\n") + self._write(b, "plugin.yml", "name: x\n") + assert _dirs_differ(a, b) is True + + def test_extra_file_in_b_differs(self, tmp_path): + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: x\n") + self._write(b, "plugin.yml", "name: x\n") + self._write(b, "README.md", "new upstream doc\n") + assert _dirs_differ(a, b) is True + + +class TestNeedsMigration: + def test_true_when_legacy_present(self, registry): + registry.add(_installed("adminer", "plugins/adminer")) + assert needs_migration(registry) is True + + def test_false_when_only_repos_and_linked(self, registry): + registry.add(_installed( + "adminer", "repos/github.com--testorg--testrepo/adminer", + )) + registry.add(_installed("local", "plugins/local", linked=True)) + assert needs_migration(registry) is False + + def test_false_when_empty(self, registry): + assert needs_migration(registry) is False + + +# ── migrate() ─────────────────────────────────────────────────── + + +class TestMigrateClean: + def test_clean_migration_updates_path_and_deletes_copy(self, registry, devbase_root): + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + + result = migrate(registry) + + # path rewritten to repos/-based location + migrated_plugin = registry.get("adminer") + assert migrated_plugin.path == f"repos/{DIRNAME}/adminer" + # old copy removed + assert not (devbase_root / "plugins" / "adminer").exists() + # result bookkeeping + assert result.migrated == ["adminer"] + assert result.preserved == [] + assert result.errors == [] + + def test_clean_migration_creates_repos_symlink(self, registry, devbase_root): + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + + migrate(registry) + + link = devbase_root / "projects" / "adminer" + assert link.is_symlink() + target = (link.parent / link.readlink()).resolve() + assert target == (devbase_root / "repos" / DIRNAME / "adminer" / "projects" / "adminer").resolve() + + def test_clean_migration_empties_plugins_dir_to_gitkeep(self, registry, devbase_root): + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + + result = migrate(registry) + + plugins_dir = devbase_root / "plugins" + remaining = sorted(p.name for p in plugins_dir.iterdir()) + assert remaining == [".gitkeep"] + assert result.plugins_dir_cleaned is True + + +class TestMigrateWithLocalChanges: + def test_diverged_copy_preserved_as_bak(self, registry, devbase_root): + plugins = [{"name": "carmo", "path": "carmo", "projects": ["carmo"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + # User added a local .env that does not exist upstream + _make_legacy_copy(devbase_root, "carmo", plugins[0], + extra={"projects/carmo/.env": "LOCAL=1\n"}) + registry.add(_installed("carmo", "plugins/carmo")) + + result = migrate(registry) + + assert result.preserved == ["carmo"] + assert result.migrated == [] + bak = devbase_root / "plugins" / "carmo.bak" + assert bak.is_dir() + assert (bak / "projects" / "carmo" / ".env").read_text() == "LOCAL=1\n" + # original copy path no longer present + assert not (devbase_root / "plugins" / "carmo").exists() + + def test_bak_retained_plugins_dir_not_cleaned(self, registry, devbase_root): + plugins = [{"name": "carmo", "path": "carmo", "projects": ["carmo"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "carmo", plugins[0], + extra={"extra.txt": "x\n"}) + registry.add(_installed("carmo", "plugins/carmo")) + + result = migrate(registry) + + assert result.plugins_dir_cleaned is False + # path is still rewritten to repos/ even when copy is preserved + assert registry.get("carmo").path == f"repos/{DIRNAME}/carmo" + + +class TestMigrateClonesMissingRepo: + def test_repo_without_local_path_is_cloned(self, registry, devbase_root): + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + # Repo registered WITHOUT local_path (legacy registration) + _register_repo(registry, plugins, local_path=None) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + + def fake_clone(url, dest, **kwargs): + _make_repo_clone(dest.parent.parent, plugins) + + with patch("devbase.plugin.installer.git_clone", side_effect=fake_clone): + result = migrate(registry) + + assert result.migrated == ["adminer"] + repo = registry.get_repository_by_url(URL) + assert repo.local_path == f"repos/{DIRNAME}" + assert registry.get("adminer").path == f"repos/{DIRNAME}/adminer" + + +class TestMigrateKeepsLinked: + def test_linked_install_keeps_plugins_dir(self, registry, devbase_root): + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + # A separate --link install lives under plugins/ and must be preserved + (devbase_root / "plugins" / "locallink").mkdir(parents=True) + registry.add(_installed("locallink", "plugins/locallink", linked=True)) + + result = migrate(registry) + + assert result.migrated == ["adminer"] + assert result.plugins_dir_cleaned is False + assert (devbase_root / "plugins" / "locallink").is_dir() + + +class TestMigrateSkips: + def test_unregistered_source_is_skipped(self, registry, devbase_root): + _make_legacy_copy( + devbase_root, "orphan", + {"name": "orphan", "path": "orphan", "projects": ["orphan"]}, + ) + registry.add(_installed("orphan", "plugins/orphan")) + + result = migrate(registry) + + assert result.skipped == ["orphan"] + assert result.migrated == [] + # untouched: copy stays, path unchanged + assert (devbase_root / "plugins" / "orphan").is_dir() + assert registry.get("orphan").path == "plugins/orphan" + + +class TestCmdPluginMigrate: + def test_command_runs_migration(self, devbase_root): + from devbase.commands.plugin import cmd_plugin_migrate + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _make_repo_clone(devbase_root, plugins) + registry = PluginRegistry(devbase_root) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + + rc = cmd_plugin_migrate(devbase_root) + + assert rc == 0 + assert PluginRegistry(devbase_root).get("adminer").path == f"repos/{DIRNAME}/adminer" + + def test_command_noop_when_nothing_to_migrate(self, devbase_root): + from devbase.commands.plugin import cmd_plugin_migrate + rc = cmd_plugin_migrate(devbase_root) + assert rc == 0 + + +class TestAutoMigrateOnInstall: + def test_install_triggers_migration_of_legacy(self, registry, devbase_root): + plugins = [ + {"name": "adminer", "path": "adminer", "projects": ["adminer"]}, + {"name": "carmo", "path": "carmo", "projects": ["carmo"]}, + ] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + + from devbase.plugin.installer import install_plugin + install_plugin(registry, "carmo") + + # pre-existing legacy install migrated as a side effect + assert registry.get("adminer").path == f"repos/{DIRNAME}/adminer" + # the explicitly requested install also succeeds + assert registry.get("carmo").path == f"repos/{DIRNAME}/carmo" + + +class TestAutoMigrateOnUpdate: + def test_update_triggers_migration_of_legacy(self, registry, devbase_root): + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + + from devbase.plugin.updater import update_plugin + with patch("devbase.plugin.updater._git_pull"): + update_plugin(registry) + + assert registry.get("adminer").path == f"repos/{DIRNAME}/adminer" + # only auto-migration removes the stale plugins/ copy; a plain pull + # would rewrite the path but leave the old copy on disk. + assert not (devbase_root / "plugins" / "adminer").exists() From 41cea7f972cb0bd497a3d68345e454070277b55e Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Thu, 28 May 2026 07:15:22 +0000 Subject: [PATCH 3/9] =?UTF-8?q?fix(plugin):=20migration=20=E3=81=AE=20syml?= =?UTF-8?q?ink=20=E5=B7=AE=E5=88=86=E6=A4=9C=E7=9F=A5=E6=BC=8F=E3=82=8C=20?= =?UTF-8?q?/=20.bak=20=E4=B8=8A=E6=9B=B8=E3=81=8D=20/=20registry=20?= =?UTF-8?q?=E5=85=88=E8=A1=8C=E6=9B=B4=E6=96=B0=E3=82=92=E4=BF=AE=E6=AD=A3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cross-review round 1 の major 指摘 4 件 (codex 2 / gemini 2) に対応。 - _dirs_differ: regular file のみ比較していたため legacy copy のみに存在する symlink / 空ディレクトリ / 型不一致を差分として検知できず、後続の shutil.rmtree で silently 削除される恐れがあった。全エントリを対象に 型 + 内容 (file は byte, symlink は target) を比較するよう厳密化 - _unique_bak_path: 既存の .bak を無条件に rmtree していたため、 前回 migration で保全した未整理バックアップが消失する恐れがあった。 存在時は .bak-2, .bak-3 ... と一意名に退避するよう変更 - migrate: filesystem の退避/削除が成功してから registry.add で plugins.yml を repos/ path に書き換えるよう順序を入れ替え。失敗時に registry だけ先行更新され retry も効かなくなる partial state を防止 - _cleanup_plugins_dir: .bak-N 形式も保全 .bak として検知するよう調整 - 上記挙動を網羅するテスト 6 件追加 Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/devbase/plugin/migrator.py | 103 +++++++++++++++++++++++---------- tests/plugin/test_migrator.py | 63 ++++++++++++++++++++ 2 files changed, 135 insertions(+), 31 deletions(-) diff --git a/lib/devbase/plugin/migrator.py b/lib/devbase/plugin/migrator.py index b9df50f..ed14665 100644 --- a/lib/devbase/plugin/migrator.py +++ b/lib/devbase/plugin/migrator.py @@ -40,32 +40,70 @@ def needs_migration(registry: PluginRegistry) -> bool: return any(_is_legacy_plugin(p) for p in registry.list_installed()) +def _unique_bak_path(old_dir: Path) -> Path: + """Return a non-existing .bak path, suffixing -2, -3, … if needed. + + A previous migration run may have already preserved .bak awaiting + manual reconciliation; never overwrite it, so each diverged copy lands in + its own directory. + """ + bak = old_dir.with_name(old_dir.name + '.bak') + if not bak.exists(): + return bak + n = 2 + while True: + candidate = old_dir.with_name(f"{old_dir.name}.bak-{n}") + if not candidate.exists(): + return candidate + n += 1 + + +def _entry_kind(p: Path) -> str: + """Classify a path for diff purposes: symlink / dir / file / other. + + Symlinks are checked first (a symlink to a dir would otherwise read as a + dir) so that a copy/clone type mismatch is always detected. + """ + if p.is_symlink(): + return 'symlink' + if p.is_dir(): + return 'dir' + if p.is_file(): + return 'file' + return 'other' + + def _dirs_differ(copy_dir: Path, repo_dir: Path) -> bool: """True if the legacy copy differs in any way from the repos/ clone dir. - Compares the set of regular files and their byte content. Any divergence - (changed content, a user-added file, or an upstream-added file) is treated - as a difference so the copy is preserved rather than deleted — migration - must never silently discard data. + Walks *every* entry (regular files, symlinks, and directories — including + empty ones) and compares both the entry set and, per entry, its type plus + file byte content / symlink target. Any divergence (changed content, a + user-added file/symlink/empty dir, a type mismatch, or an upstream-added + entry) is treated as a difference so the copy is preserved rather than + deleted — migration must never silently discard data. """ - def _files(base: Path) -> set[Path]: - return { - p.relative_to(base) - for p in base.rglob('*') - if p.is_file() and not p.is_symlink() - } - - copy_files = _files(copy_dir) - repo_files = _files(repo_dir) - if copy_files != repo_files: + def _entries(base: Path) -> set[Path]: + return {p.relative_to(base) for p in base.rglob('*')} + + copy_entries = _entries(copy_dir) + repo_entries = _entries(repo_dir) + if copy_entries != repo_entries: return True - for rel in copy_files: + for rel in copy_entries: fa, fb = copy_dir / rel, repo_dir / rel - if fa.stat().st_size != fb.stat().st_size: - return True - if fa.read_bytes() != fb.read_bytes(): + kind = _entry_kind(fa) + if kind != _entry_kind(fb): return True + if kind == 'symlink': + if fa.readlink() != fb.readlink(): + return True + elif kind == 'file': + if fa.stat().st_size != fb.stat().st_size: + return True + if fa.read_bytes() != fb.read_bytes(): + return True return False @@ -134,7 +172,7 @@ def _cleanup_plugins_dir(registry: PluginRegistry) -> bool: return False entries = [e for e in plugins_dir.iterdir() if e.name != '.gitkeep'] - bak_dirs = [e for e in entries if e.name.endswith('.bak')] + bak_dirs = [e for e in entries if '.bak' in e.name] if bak_dirs: logger.info( "plugins/ retained: %d preserved .bak dir(s) await manual reconciliation", @@ -202,21 +240,15 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu info = load_plugin_info(repo_plugin_dir) version = info.version if info else plugin.version - registry.add(InstalledPlugin( - name=plugin.name, - version=version, - source=plugin.source, - installed_at=plugin.installed_at, - path=rel_path, - linked=False, - )) - + # Retire the old plugins/ copy FIRST, then update the registry. + # Updating plugins.yml before the filesystem move means a failure + # here would leave registry at repos/ (no longer "legacy", so never + # retried) while the stale copy lingers — so the path rewrite must + # only be committed once the copy has been removed or preserved. old_dir = registry.devbase_root / plugin.path if old_dir.is_dir() and not old_dir.is_symlink(): if _dirs_differ(old_dir, repo_plugin_dir): - bak = old_dir.with_name(old_dir.name + '.bak') - if bak.exists(): - shutil.rmtree(bak) + bak = _unique_bak_path(old_dir) old_dir.rename(bak) result.preserved.append(plugin.name) logger.warning( @@ -229,6 +261,15 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu result.migrated.append(plugin.name) else: result.migrated.append(plugin.name) + + registry.add(InstalledPlugin( + name=plugin.name, + version=version, + source=plugin.source, + installed_at=plugin.installed_at, + path=rel_path, + linked=False, + )) except Exception as e: result.skipped.append(plugin.name) result.errors.append(f"{plugin.name}: {e}") diff --git a/tests/plugin/test_migrator.py b/tests/plugin/test_migrator.py index fc692c4..2a4db16 100644 --- a/tests/plugin/test_migrator.py +++ b/tests/plugin/test_migrator.py @@ -158,6 +158,47 @@ def test_extra_file_in_b_differs(self, tmp_path): self._write(b, "README.md", "new upstream doc\n") assert _dirs_differ(a, b) is True + def test_extra_symlink_in_copy_differs(self, tmp_path): + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: x\n") + self._write(b, "plugin.yml", "name: x\n") + # User-added symlink present only in the legacy copy + (a / "link").symlink_to("plugin.yml") + assert _dirs_differ(a, b) is True + + def test_empty_dir_only_in_copy_differs(self, tmp_path): + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: x\n") + self._write(b, "plugin.yml", "name: x\n") + # User-added empty directory present only in the legacy copy + (a / "logs").mkdir(parents=True) + assert _dirs_differ(a, b) is True + + def test_symlink_target_change_differs(self, tmp_path): + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: x\n") + self._write(b, "plugin.yml", "name: x\n") + (a / "link").symlink_to("a-target") + (b / "link").symlink_to("b-target") + assert _dirs_differ(a, b) is True + + def test_file_vs_symlink_type_mismatch_differs(self, tmp_path): + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: x\n") + self._write(b, "plugin.yml", "name: x\n") + # Same name, but a regular file in the copy vs a symlink in the clone + self._write(a, "shared", "data\n") + (b / "shared").symlink_to("plugin.yml") + assert _dirs_differ(a, b) is True + + def test_identical_symlinks_do_not_differ(self, tmp_path): + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: x\n") + self._write(b, "plugin.yml", "name: x\n") + (a / "link").symlink_to("plugin.yml") + (b / "link").symlink_to("plugin.yml") + assert _dirs_differ(a, b) is False + class TestNeedsMigration: def test_true_when_legacy_present(self, registry): @@ -261,6 +302,28 @@ def test_bak_retained_plugins_dir_not_cleaned(self, registry, devbase_root): # path is still rewritten to repos/ even when copy is preserved assert registry.get("carmo").path == f"repos/{DIRNAME}/carmo" + def test_existing_bak_is_not_overwritten(self, registry, devbase_root): + plugins = [{"name": "carmo", "path": "carmo", "projects": ["carmo"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "carmo", plugins[0], + extra={"projects/carmo/.env": "LOCAL=1\n"}) + registry.add(_installed("carmo", "plugins/carmo")) + # A previous migration run already preserved carmo.bak with its own data + prev_bak = devbase_root / "plugins" / "carmo.bak" + prev_bak.mkdir(parents=True) + (prev_bak / "old.txt").write_text("PREVIOUS\n") + + result = migrate(registry) + + assert result.preserved == ["carmo"] + # the old .bak survives untouched + assert (prev_bak / "old.txt").read_text() == "PREVIOUS\n" + # the new diverged copy lands in a distinct .bak-2 dir + new_bak = devbase_root / "plugins" / "carmo.bak-2" + assert new_bak.is_dir() + assert (new_bak / "projects" / "carmo" / ".env").read_text() == "LOCAL=1\n" + class TestMigrateClonesMissingRepo: def test_repo_without_local_path_is_cloned(self, registry, devbase_root): From 98cfb6570a98a322d46071ed27bf4fb3ae773aa9 Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Thu, 28 May 2026 07:29:25 +0000 Subject: [PATCH 4/9] =?UTF-8?q?fix(plugin):=20migration=20=E3=81=AE?= =?UTF-8?q?=E5=B7=AE=E5=88=86=E5=88=A4=E5=AE=9A=E5=8E=B3=E5=AF=86=E5=8C=96?= =?UTF-8?q?=20/=20partial=20clone=20=E5=BE=A9=E6=97=A7=20/=20cleanup=20?= =?UTF-8?q?=E5=A0=B1=E5=91=8A=E4=BF=AE=E6=AD=A3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cross-review round 2 の指摘対応。 - _dirs_differ: upstream 専用追加 (clone にのみ存在) を差分扱いしないよう 変更。コピー側にのみ存在するエントリ・共通エントリの型/target/内容差分の みを preserved 判定に使い、通常の upstream 更新で不要な .bak 退避が発生 しないようにした (codex#91 / gemini#91 重複指摘)。 - _files_equal: read_bytes() の全読み込みを 64KB チャンクのストリーム比較に 置き換え、巨大ファイルでのメモリ枯渇リスクを排除 (gemini#105)。 - _ensure_repo_cloned: 前回 clone 失敗で残った partial dir (.git 無し / registry.yml 不正) を検知して削除・再 clone するよう修正。無限に parse 失敗を繰り返す経路を解消 (codex#132)。 - _cleanup_plugins_dir: .gitkeep でも .bak でもない想定外エントリが残る場合 は cleaned=True と報告せず False を返すよう修正 (gemini#176)。 - docs: devbase plugin migrate の CLI リファレンスを追加 (codex review body)。 テスト 4 件追加 (upstream 専用追加は差分なし / 同サイズ内容差 / partial clone 再 clone / cleanup の想定外エントリ保持)。全 252 件 pass。 Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/user/cli-reference.md | 18 +++++++++ lib/devbase/plugin/migrator.py | 68 ++++++++++++++++++++++++++++------ tests/plugin/test_migrator.py | 55 ++++++++++++++++++++++++++- 3 files changed, 128 insertions(+), 13 deletions(-) diff --git a/docs/user/cli-reference.md b/docs/user/cli-reference.md index 74d0bf6..d099d0a 100644 --- a/docs/user/cli-reference.md +++ b/docs/user/cli-reference.md @@ -404,6 +404,24 @@ devbase plugin info devbase plugin sync ``` +### `devbase plugin migrate` + +旧形式 (`plugins/` へのコピー) でインストールされたプラグインを、`repos/` 配下の永続クローンへ移行します。`install` / `update` 実行時にも自動で呼び出されるため、通常は手動実行不要です。 + +``` +devbase plugin migrate +``` + +移行の挙動: + +| 状況 | 動作 | +|---|---| +| コピーがクローンと一致 | 旧コピーを削除し `repos/` へ移行 (migrated) | +| コピーにローカル変更あり | 旧コピーを `plugins/.bak` として保全 (preserved、手動で reconcile) | +| 移行できない (ソース未登録 等) | スキップしてエラーを表示 (skipped) | + +`--link` でインストールしたプラグインは移行対象外です。 + ### `devbase plugin repo add` プラグインリポジトリを登録します。 diff --git a/lib/devbase/plugin/migrator.py b/lib/devbase/plugin/migrator.py index ed14665..51fd692 100644 --- a/lib/devbase/plugin/migrator.py +++ b/lib/devbase/plugin/migrator.py @@ -73,22 +73,43 @@ def _entry_kind(p: Path) -> str: return 'other' +def _files_equal(fa: Path, fb: Path) -> bool: + """Compare two regular files by size then streamed byte content. + + Reads in fixed-size chunks rather than slurping the whole file so a large + plugin asset can't exhaust memory during the migration scan. + """ + if fa.stat().st_size != fb.stat().st_size: + return False + chunk = 64 * 1024 + with fa.open('rb') as a, fb.open('rb') as b: + while True: + ba, bb = a.read(chunk), b.read(chunk) + if ba != bb: + return False + if not ba: + return True + + def _dirs_differ(copy_dir: Path, repo_dir: Path) -> bool: - """True if the legacy copy differs in any way from the repos/ clone dir. + """True if deleting the legacy copy would discard data not in the clone. Walks *every* entry (regular files, symlinks, and directories — including - empty ones) and compares both the entry set and, per entry, its type plus - file byte content / symlink target. Any divergence (changed content, a - user-added file/symlink/empty dir, a type mismatch, or an upstream-added - entry) is treated as a difference so the copy is preserved rather than - deleted — migration must never silently discard data. + empty ones). An entry is treated as divergence only when it represents + data the copy holds but the clone does not: a copy-only entry (user-added + file/symlink/empty dir), or a common entry whose type, symlink target, or + file content differs. Upstream-only additions (present in the clone but + not the copy) are *not* a difference — deleting the copy loses nothing — so + a routine upstream change no longer forces a manual-reconcile .bak. """ def _entries(base: Path) -> set[Path]: return {p.relative_to(base) for p in base.rglob('*')} copy_entries = _entries(copy_dir) repo_entries = _entries(repo_dir) - if copy_entries != repo_entries: + + # User-added entries live only in the copy and would be lost on delete. + if copy_entries - repo_entries: return True for rel in copy_entries: @@ -100,9 +121,7 @@ def _entries(base: Path) -> set[Path]: if fa.readlink() != fb.readlink(): return True elif kind == 'file': - if fa.stat().st_size != fb.stat().st_size: - return True - if fa.read_bytes() != fb.read_bytes(): + if not _files_equal(fa, fb): return True return False @@ -129,11 +148,28 @@ def _ensure_repo_cloned( repos_dir.mkdir(exist_ok=True) clone_dir = repos_dir / dir_name + # A leftover directory from a previously interrupted clone (e.g. disk full + # or network drop) would otherwise be reused forever — re-cloning is + # skipped while parse_registry_yml() keeps failing, so migration can never + # self-heal. Treat a dir that is not a valid clone (missing .git) as + # broken and remove it so the clone below re-creates it cleanly. + if clone_dir.is_dir() and not (clone_dir / '.git').exists(): + shutil.rmtree(clone_dir) + if not clone_dir.is_dir(): - git_clone(repo.url, clone_dir, shallow=False) + try: + git_clone(repo.url, clone_dir, shallow=False) + except Exception: + # Drop a partial clone so the next run starts from a clean slate. + if clone_dir.is_dir(): + shutil.rmtree(clone_dir) + raise reg_info = parse_registry_yml(clone_dir) if not reg_info: + # The clone exists but is missing/invalid registry.yml; discard it so + # a subsequent migration attempt re-clones instead of looping here. + shutil.rmtree(clone_dir) raise PluginError( f"No registry.yml found in cloned repository '{repo.name}'." ) @@ -180,6 +216,16 @@ def _cleanup_plugins_dir(registry: PluginRegistry) -> bool: ) return False + # Anything left over that is neither .gitkeep nor a preserved .bak means + # plugins/ is not actually clean; leave it untouched and report uncleaned. + leftover = [e for e in entries if e not in bak_dirs] + if leftover: + logger.info( + "plugins/ retained: %d unexpected entr(y/ies) remain (%s)", + len(leftover), ", ".join(sorted(e.name for e in leftover)), + ) + return False + gitkeep = plugins_dir / '.gitkeep' if not gitkeep.exists(): gitkeep.touch() diff --git a/tests/plugin/test_migrator.py b/tests/plugin/test_migrator.py index 2a4db16..7bc1df9 100644 --- a/tests/plugin/test_migrator.py +++ b/tests/plugin/test_migrator.py @@ -15,6 +15,7 @@ ) from devbase.plugin.registry import PluginRegistry from devbase.plugin.migrator import ( + _cleanup_plugins_dir, _dirs_differ, _is_legacy_plugin, migrate, @@ -144,6 +145,14 @@ def test_changed_file_content_differs(self, tmp_path): self._write(b, "plugin.yml", "name: x\n") assert _dirs_differ(a, b) is True + def test_same_size_different_content_differs(self, tmp_path): + # Same byte length but different content — exercises the streamed + # chunk comparison rather than the size fast-path. + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "plugin.yml", "name: aaaa\n") + self._write(b, "plugin.yml", "name: bbbb\n") + assert _dirs_differ(a, b) is True + def test_extra_file_in_a_differs(self, tmp_path): a, b = tmp_path / "a", tmp_path / "b" self._write(a, "plugin.yml", "name: x\n") @@ -151,12 +160,15 @@ def test_extra_file_in_a_differs(self, tmp_path): self._write(b, "plugin.yml", "name: x\n") assert _dirs_differ(a, b) is True - def test_extra_file_in_b_differs(self, tmp_path): + def test_upstream_only_addition_does_not_differ(self, tmp_path): + # A file present only upstream (in the clone) is not data the legacy + # copy holds, so deleting the copy loses nothing — a routine upstream + # addition must NOT force a manual-reconcile .bak. a, b = tmp_path / "a", tmp_path / "b" self._write(a, "plugin.yml", "name: x\n") self._write(b, "plugin.yml", "name: x\n") self._write(b, "README.md", "new upstream doc\n") - assert _dirs_differ(a, b) is True + assert _dirs_differ(a, b) is False def test_extra_symlink_in_copy_differs(self, tmp_path): a, b = tmp_path / "a", tmp_path / "b" @@ -380,6 +392,45 @@ def test_unregistered_source_is_skipped(self, registry, devbase_root): assert registry.get("orphan").path == "plugins/orphan" +class TestCleanupPluginsDir: + def test_unexpected_leftover_keeps_plugins_dir_uncleaned(self, registry, devbase_root): + # plugins/ holds a stray entry that is neither .gitkeep nor a .bak + # (e.g. left behind by an external tool); cleanup must not claim it + # cleaned and must leave the entry in place. + plugins_dir = devbase_root / "plugins" + plugins_dir.mkdir() + (plugins_dir / "stray").mkdir() + + assert _cleanup_plugins_dir(registry) is False + assert (plugins_dir / "stray").is_dir() + + +class TestMigratePartialCloneRecovery: + def test_partial_clone_without_git_is_recloned(self, registry, devbase_root): + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + # Repo registered without local_path so migrate() takes the clone path. + _register_repo(registry, plugins, local_path=None) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + # Leftover partial clone from a prior interrupted run: a directory with + # no .git and no registry.yml that previously caused an infinite loop. + partial = devbase_root / "repos" / DIRNAME + partial.mkdir(parents=True) + (partial / "junk.txt").write_text("partial\n") + + def fake_clone(url, dest, **kwargs): + _make_repo_clone(dest.parent.parent, plugins) + + with patch("devbase.plugin.installer.git_clone", side_effect=fake_clone) as mock: + result = migrate(registry) + + # The broken dir was removed and a fresh clone performed. + mock.assert_called_once() + assert result.migrated == ["adminer"] + assert registry.get("adminer").path == f"repos/{DIRNAME}/adminer" + assert not (devbase_root / "repos" / DIRNAME / "junk.txt").exists() + + class TestCmdPluginMigrate: def test_command_runs_migration(self, devbase_root): from devbase.commands.plugin import cmd_plugin_migrate From 04f0d0242d26cc3d3c6e8811e4dffdd93b1c8afb Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Thu, 28 May 2026 11:24:48 +0000 Subject: [PATCH 5/9] =?UTF-8?q?fix(plugin):=20migration=20=E3=81=AE?= =?UTF-8?q?=E4=BF=9D=E5=85=A8=E5=88=A4=E5=AE=9A=E3=83=BBclone=20=E5=81=A5?= =?UTF-8?q?=E5=85=A8=E6=80=A7=E3=83=BBregistry=20=E4=BF=9D=E5=AD=98?= =?UTF-8?q?=E5=8A=B9=E7=8E=87=E3=82=92=E6=94=B9=E5=96=84=20(PR2=20round3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cross-review round 3 の指摘に対応: - _cleanup_plugins_dir の `.bak` 判定を `'.bak' in name` から `.bak[-N]` 末尾一致 (_is_bak_name) に修正。my.bakery 等の誤マッチを排除 - migrate ループ内の per-plugin `registry.add` を loop 末尾の単一 `registry.add_many` に集約し plugins.yml の保存頻発を解消 (各 plugin の fs 移動と entry 構築は同一 try 内のため失敗時の retry 性は維持) - _ensure_repo_cloned で local_path 設定済みでも .git/registry.yml を 検証 (_clone_is_healthy)。壊れた既存 dir は除去して再 clone - _files_equal で S_IMODE を比較。旧コピーの実行ビット変更を差分扱いし保全 - _auto_migrate の preserved/skipped 再通知を loud な per-plugin WARNING から 簡潔な INFO ヒント 1 行に抑制 (詳細は devbase plugin migrate 側で出力) migrator テスト 12 件追加 (.bak 末尾判定 / clone 健全性 / 実行ビット差分 / batched save / broken local_path 再 clone / 警告抑制)。全 264 件 pass。 Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/devbase/plugin/installer.py | 15 +-- lib/devbase/plugin/migrator.py | 69 +++++++++++-- lib/devbase/plugin/registry.py | 15 ++- tests/plugin/test_migrator.py | 165 ++++++++++++++++++++++++++++++++ 4 files changed, 247 insertions(+), 17 deletions(-) diff --git a/lib/devbase/plugin/installer.py b/lib/devbase/plugin/installer.py index 5cb157a..96ffb6d 100644 --- a/lib/devbase/plugin/installer.py +++ b/lib/devbase/plugin/installer.py @@ -95,13 +95,16 @@ def _auto_migrate(registry: PluginRegistry) -> None: result = migrate(registry) if result.migrated: logger.info(" Migrated: %s", ", ".join(result.migrated)) - if result.preserved: - logger.warning( - " Preserved with local changes (plugins/.bak): %s", - ", ".join(result.preserved), + # preserved/skipped recur on every install/update until the user + # reconciles, so avoid re-emitting a loud per-plugin WARNING each time: + # surface a single concise hint pointing at the explicit command, which + # prints the full per-plugin detail when run. + if result.preserved or result.skipped: + pending = len(result.preserved) + len(result.skipped) + logger.info( + " %d plugin(s) still need attention — run 'devbase plugin migrate' " + "for details.", pending, ) - if result.skipped: - logger.warning(" Could not migrate: %s", ", ".join(result.skipped)) def install_plugin( diff --git a/lib/devbase/plugin/migrator.py b/lib/devbase/plugin/migrator.py index 51fd692..0516beb 100644 --- a/lib/devbase/plugin/migrator.py +++ b/lib/devbase/plugin/migrator.py @@ -1,6 +1,8 @@ """Plugin migrator - migrates legacy plugins/ copy installs to repos/ clones""" +import re import shutil +import stat from dataclasses import dataclass, field from pathlib import Path @@ -40,6 +42,19 @@ def needs_migration(registry: PluginRegistry) -> bool: return any(_is_legacy_plugin(p) for p in registry.list_installed()) +# Names produced by _unique_bak_path: ".bak" or ".bak-". +_BAK_NAME_RE = re.compile(r'\.bak(-\d+)?$') + + +def _is_bak_name(name: str) -> bool: + """True if name matches the preserved-copy convention (.bak[-N]). + + A substring check like ``'.bak' in name`` would wrongly flag unrelated + entries such as ``my.bakery`` or ``notes.bak.txt``; anchor to the suffix. + """ + return _BAK_NAME_RE.search(name) is not None + + def _unique_bak_path(old_dir: Path) -> Path: """Return a non-existing .bak path, suffixing -2, -3, … if needed. @@ -74,12 +89,18 @@ def _entry_kind(p: Path) -> str: def _files_equal(fa: Path, fb: Path) -> bool: - """Compare two regular files by size then streamed byte content. + """Compare two regular files by permission bits, size, then byte content. Reads in fixed-size chunks rather than slurping the whole file so a large - plugin asset can't exhaust memory during the migration scan. + plugin asset can't exhaust memory during the migration scan. Permission + bits (S_IMODE) are compared too so a local exec-bit change (e.g. an entry + script the user made executable) counts as divergence and is preserved + rather than silently lost when the copy is deleted. """ - if fa.stat().st_size != fb.stat().st_size: + sa, sb = fa.stat(), fb.stat() + if stat.S_IMODE(sa.st_mode) != stat.S_IMODE(sb.st_mode): + return False + if sa.st_size != sb.st_size: return False chunk = 64 * 1024 with fa.open('rb') as a, fb.open('rb') as b: @@ -126,22 +147,42 @@ def _entries(base: Path) -> set[Path]: return False +def _clone_is_healthy(clone_dir: Path) -> bool: + """True if clone_dir looks like a usable repo clone (has .git + registry.yml). + + A repos/ dir that lost its .git (or whose registry.yml went missing) points + migrated plugins at a tree that can never be pulled/updated again, so it + must be re-cloned rather than reused. + """ + return ( + clone_dir.is_dir() + and (clone_dir / '.git').exists() + and (clone_dir / 'registry.yml').is_file() + ) + + def _ensure_repo_cloned( registry: PluginRegistry, repo: RegisteredRepository, ) -> tuple[Path, RegisteredRepository]: """Return the repos/ clone dir for a repo, cloning it if necessary. - If the repo was registered before persistent-clone support (no local_path) - or the clone dir is missing, perform a full clone and persist local_path + - a refreshed plugin list to plugins.yml. + If the repo was registered before persistent-clone support (no local_path), + the clone dir is missing, or the existing clone is broken (missing .git / + registry.yml), perform a full clone and persist local_path + a refreshed + plugin list to plugins.yml. """ from .installer import git_clone, parse_registry_yml from .repo_manager import _url_to_repos_dirname if repo.local_path: clone_dir = registry.devbase_root / repo.local_path - if clone_dir.is_dir(): + if _clone_is_healthy(clone_dir): return clone_dir, repo + # Recorded local_path exists but is not a valid clone (.git lost, etc.): + # drop it so the re-clone below repairs the tree instead of returning a + # path that update/pull can never recover. + if clone_dir.is_dir(): + shutil.rmtree(clone_dir) dir_name = _url_to_repos_dirname(repo.url) repos_dir = registry.get_repos_dir() @@ -208,7 +249,7 @@ def _cleanup_plugins_dir(registry: PluginRegistry) -> bool: return False entries = [e for e in plugins_dir.iterdir() if e.name != '.gitkeep'] - bak_dirs = [e for e in entries if '.bak' in e.name] + bak_dirs = [e for e in entries if _is_bak_name(e.name)] if bak_dirs: logger.info( "plugins/ retained: %d preserved .bak dir(s) await manual reconciliation", @@ -248,6 +289,13 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu if not legacy: return result + # Accumulate path rewrites and persist them in a single plugins.yml save + # after the loop instead of one atomic rewrite per plugin. Each plugin's + # filesystem move and its registry entry are built inside the same try + # block, so a failure leaves that plugin out of `pending` (still legacy -> + # retried next run) while completed ones are still committed together. + pending: list[InstalledPlugin] = [] + for plugin in legacy: try: repo = registry.get_repository_by_url(plugin.source) @@ -308,7 +356,7 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu else: result.migrated.append(plugin.name) - registry.add(InstalledPlugin( + pending.append(InstalledPlugin( name=plugin.name, version=version, source=plugin.source, @@ -320,6 +368,9 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu result.skipped.append(plugin.name) result.errors.append(f"{plugin.name}: {e}") + # Single save for every plugin whose copy was successfully retired. + registry.add_many(pending) + if run_sync: sync_projects(registry) diff --git a/lib/devbase/plugin/registry.py b/lib/devbase/plugin/registry.py index f852fcb..f3cc58d 100644 --- a/lib/devbase/plugin/registry.py +++ b/lib/devbase/plugin/registry.py @@ -65,11 +65,22 @@ def get(self, name: str) -> Optional[InstalledPlugin]: def add(self, plugin: InstalledPlugin) -> None: """Add a plugin to the registry""" + self.add_many([plugin]) + + def add_many(self, plugins: list[InstalledPlugin]) -> None: + """Add/replace multiple plugins with a single load+save. + + Saving plugins.yml once for a batch avoids the repeated read+atomic + rewrite that calling add() per plugin would incur (e.g. during + migration of many legacy installs).""" + if not plugins: + return data = self._load() + names = {p.name for p in plugins} data['installed_plugins'] = [ - p for p in data['installed_plugins'] if p['name'] != plugin.name + p for p in data['installed_plugins'] if p['name'] not in names ] - data['installed_plugins'].append(plugin.to_dict()) + data['installed_plugins'].extend(p.to_dict() for p in plugins) self._save(data) def remove(self, name: str) -> bool: diff --git a/tests/plugin/test_migrator.py b/tests/plugin/test_migrator.py index 7bc1df9..a3a8557 100644 --- a/tests/plugin/test_migrator.py +++ b/tests/plugin/test_migrator.py @@ -16,7 +16,9 @@ from devbase.plugin.registry import PluginRegistry from devbase.plugin.migrator import ( _cleanup_plugins_dir, + _clone_is_healthy, _dirs_differ, + _is_bak_name, _is_legacy_plugin, migrate, needs_migration, @@ -211,6 +213,61 @@ def test_identical_symlinks_do_not_differ(self, tmp_path): (b / "link").symlink_to("plugin.yml") assert _dirs_differ(a, b) is False + def test_exec_bit_change_differs(self, tmp_path): + # Same name, size and bytes, but the copy made the script executable. + # Deleting the copy would lose that mode change, so it must count as + # divergence (preserved as .bak) rather than a clean migration. + a, b = tmp_path / "a", tmp_path / "b" + self._write(a, "entry.sh", "#!/bin/sh\necho hi\n") + self._write(b, "entry.sh", "#!/bin/sh\necho hi\n") + (b / "entry.sh").chmod(0o644) + (a / "entry.sh").chmod(0o755) + # Sanity: bytes identical, only mode differs. + assert (a / "entry.sh").read_bytes() == (b / "entry.sh").read_bytes() + assert _dirs_differ(a, b) is True + # And once the modes match, the copy is treated as identical again. + (a / "entry.sh").chmod(0o644) + assert _dirs_differ(a, b) is False + + +class TestIsBakName: + def test_plain_bak_matches(self): + assert _is_bak_name("carmo.bak") is True + + def test_numbered_bak_matches(self): + assert _is_bak_name("carmo.bak-2") is True + assert _is_bak_name("carmo.bak-17") is True + + def test_substring_bak_does_not_match(self): + # The previous `'.bak' in name` check wrongly flagged these. + assert _is_bak_name("my.bakery") is False + assert _is_bak_name("notes.bak.txt") is False + assert _is_bak_name("backup") is False + + +class TestCloneIsHealthy: + def test_valid_clone_is_healthy(self, devbase_root): + plugins = [{"name": "adminer", "path": "adminer"}] + clone = _make_repo_clone(devbase_root, plugins) + assert _clone_is_healthy(clone) is True + + def test_missing_git_is_unhealthy(self, devbase_root): + plugins = [{"name": "adminer", "path": "adminer"}] + clone = _make_repo_clone(devbase_root, plugins) + shutil_rmtree_git(clone) + assert _clone_is_healthy(clone) is False + + def test_missing_registry_yml_is_unhealthy(self, devbase_root): + plugins = [{"name": "adminer", "path": "adminer"}] + clone = _make_repo_clone(devbase_root, plugins) + (clone / "registry.yml").unlink() + assert _clone_is_healthy(clone) is False + + +def shutil_rmtree_git(clone: Path) -> None: + import shutil + shutil.rmtree(clone / ".git") + class TestNeedsMigration: def test_true_when_legacy_present(self, registry): @@ -404,6 +461,29 @@ def test_unexpected_leftover_keeps_plugins_dir_uncleaned(self, registry, devbase assert _cleanup_plugins_dir(registry) is False assert (plugins_dir / "stray").is_dir() + def test_bak_lookalike_is_not_treated_as_preserved(self, registry, devbase_root): + # An entry whose name merely contains ".bak" as a substring (e.g. + # "my.bakery") is NOT a preserved copy; it must be reported as an + # unexpected leftover rather than silently retained as a .bak. + plugins_dir = devbase_root / "plugins" + plugins_dir.mkdir() + (plugins_dir / "my.bakery").mkdir() + + assert _cleanup_plugins_dir(registry) is False + # Still present (cleanup never deletes leftovers) and not mistaken for + # a .bak dir. + assert (plugins_dir / "my.bakery").is_dir() + + def test_numbered_bak_dir_is_retained(self, registry, devbase_root): + # A real preserved copy from a prior run (carmo.bak-2) keeps plugins/ + # uncleaned just like carmo.bak does. + plugins_dir = devbase_root / "plugins" + plugins_dir.mkdir() + (plugins_dir / "carmo.bak-2").mkdir() + + assert _cleanup_plugins_dir(registry) is False + assert (plugins_dir / "carmo.bak-2").is_dir() + class TestMigratePartialCloneRecovery: def test_partial_clone_without_git_is_recloned(self, registry, devbase_root): @@ -430,6 +510,56 @@ def fake_clone(url, dest, **kwargs): assert registry.get("adminer").path == f"repos/{DIRNAME}/adminer" assert not (devbase_root / "repos" / DIRNAME / "junk.txt").exists() + def test_registered_local_path_without_git_is_recloned(self, registry, devbase_root): + # local_path is recorded (repo migrated before) but the clone lost its + # .git — e.g. an interrupted operation. Reusing it would leave the + # migrated plugin pointing at an un-pullable tree, so it must re-clone. + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _register_repo(registry, plugins) # local_path = repos/ + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + # Existing repos/ dir that is NOT a valid clone (no .git, no registry.yml). + broken = devbase_root / "repos" / DIRNAME + broken.mkdir(parents=True) + (broken / "leftover.txt").write_text("broken\n") + + def fake_clone(url, dest, **kwargs): + _make_repo_clone(dest.parent.parent, plugins) + + with patch("devbase.plugin.installer.git_clone", side_effect=fake_clone) as mock: + result = migrate(registry) + + mock.assert_called_once() + assert result.migrated == ["adminer"] + assert (devbase_root / "repos" / DIRNAME / ".git").exists() + assert not (devbase_root / "repos" / DIRNAME / "leftover.txt").exists() + + +class TestMigrateBatchesRegistryWrites: + def test_multiple_plugins_all_persisted_with_single_save(self, registry, devbase_root): + plugins = [ + {"name": "adminer", "path": "adminer", "projects": ["adminer"]}, + {"name": "carmo", "path": "carmo", "projects": ["carmo"]}, + {"name": "redis", "path": "redis", "projects": ["redis"]}, + ] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + for p in plugins: + _make_legacy_copy(devbase_root, p["name"], p) + registry.add(_installed(p["name"], f"plugins/{p['name']}")) + + with patch.object( + PluginRegistry, "_save", autospec=True, side_effect=PluginRegistry._save, + ) as save_spy: + result = migrate(registry) + + assert sorted(result.migrated) == ["adminer", "carmo", "redis"] + # All three path rewrites land in a single plugins.yml save rather than + # one save per plugin. + assert save_spy.call_count == 1 + for p in plugins: + assert registry.get(p["name"]).path == f"repos/{DIRNAME}/{p['name']}" + class TestCmdPluginMigrate: def test_command_runs_migration(self, devbase_root): @@ -472,6 +602,41 @@ def test_install_triggers_migration_of_legacy(self, registry, devbase_root): assert registry.get("carmo").path == f"repos/{DIRNAME}/carmo" +class TestAutoMigrateWarningSuppression: + def test_preserved_does_not_emit_loud_per_plugin_warning( + self, registry, devbase_root, caplog, + ): + # A diverged legacy copy is preserved as .bak. _auto_migrate runs on + # every install/update; it must not re-emit a loud per-plugin WARNING + # each time — a concise INFO hint pointing at `devbase plugin migrate` + # is enough (the explicit command prints the full detail). + plugins = [ + {"name": "carmo", "path": "carmo", "projects": ["carmo"]}, + {"name": "redis", "path": "redis", "projects": ["redis"]}, + ] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + # carmo diverges (user-added file) -> will be preserved, not migrated. + _make_legacy_copy(devbase_root, "carmo", plugins[0], + extra={"projects/carmo/.env": "LOCAL=1\n"}) + registry.add(_installed("carmo", "plugins/carmo")) + + from devbase.plugin.installer import _auto_migrate + import logging + with caplog.at_level(logging.INFO, logger="devbase.plugin.installer"): + _auto_migrate(registry) + + installer_logs = [ + r for r in caplog.records + if r.name == "devbase.plugin.installer" + ] + # No WARNING-level record from the auto path. + assert all(r.levelno < logging.WARNING for r in installer_logs) + # The concise hint is present. + joined = " ".join(r.getMessage() for r in installer_logs) + assert "devbase plugin migrate" in joined + + class TestAutoMigrateOnUpdate: def test_update_triggers_migration_of_legacy(self, registry, devbase_root): plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] From bd2afd72af617bc4da6ff2c1544a1cec62458227 Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Thu, 28 May 2026 11:38:53 +0000 Subject: [PATCH 6/9] =?UTF-8?q?fix(plugin):=20migration=20=E3=81=AE=20regi?= =?UTF-8?q?stry=20=E9=87=8D=E8=A4=87=E6=8E=92=E9=99=A4=20/=20exec=20?= =?UTF-8?q?=E3=83=93=E3=83=83=E3=83=88=E9=99=90=E5=AE=9A=E6=AF=94=E8=BC=83?= =?UTF-8?q?=20/=20=E5=81=A5=E5=85=A8=20clone=20=E4=BF=9D=E5=85=A8=20(PR2?= =?UTF-8?q?=20round4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - registry.add_many: 引数内で名前が重複する場合 last-wins で一意化してから 反映し、plugins.yml に矛盾エントリが残らないようにした - _files_equal: 全権限ビット比較を exec ビット (+x) 限定に変更し、umask / group 設定差による誤った .bak 退避を防止 - _ensure_repo_cloned: local_path 記録済みだが unhealthy な既存 dir でも .git があれば未コミット/未 push のローカル変更を失わないよう rmtree せず PluginError を送出し、.git 欠落 (真に壊れている) 場合のみ再 clone する test: add_many 重複排除 / exec ビット限定 / .git 付き clone 保全の 6 件を追加 Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/devbase/plugin/migrator.py | 35 +++++++--- lib/devbase/plugin/registry.py | 13 +++- tests/plugin/test_migrator.py | 114 +++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 13 deletions(-) diff --git a/lib/devbase/plugin/migrator.py b/lib/devbase/plugin/migrator.py index 0516beb..5907af3 100644 --- a/lib/devbase/plugin/migrator.py +++ b/lib/devbase/plugin/migrator.py @@ -88,17 +88,21 @@ def _entry_kind(p: Path) -> str: return 'other' +_EXEC_BITS = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH + + def _files_equal(fa: Path, fb: Path) -> bool: - """Compare two regular files by permission bits, size, then byte content. + """Compare two regular files by exec bits, size, then byte content. Reads in fixed-size chunks rather than slurping the whole file so a large - plugin asset can't exhaust memory during the migration scan. Permission - bits (S_IMODE) are compared too so a local exec-bit change (e.g. an entry - script the user made executable) counts as divergence and is preserved - rather than silently lost when the copy is deleted. + plugin asset can't exhaust memory during the migration scan. Only the + execute bits are compared (not the full S_IMODE): a local exec-bit change + (e.g. an entry script the user made executable) is functionally meaningful + and must be preserved, but read/write permission differences caused by the + environment's umask or group settings should not spuriously force a .bak. """ sa, sb = fa.stat(), fb.stat() - if stat.S_IMODE(sa.st_mode) != stat.S_IMODE(sb.st_mode): + if (sa.st_mode & _EXEC_BITS) != (sb.st_mode & _EXEC_BITS): return False if sa.st_size != sb.st_size: return False @@ -178,11 +182,22 @@ def _ensure_repo_cloned( clone_dir = registry.devbase_root / repo.local_path if _clone_is_healthy(clone_dir): return clone_dir, repo - # Recorded local_path exists but is not a valid clone (.git lost, etc.): - # drop it so the re-clone below repairs the tree instead of returning a - # path that update/pull can never recover. if clone_dir.is_dir(): - shutil.rmtree(clone_dir) + # The dir exists but isn't fully healthy. Only destroy it when its + # .git is gone (an un-pullable tree that re-cloning must repair). + # If .git is still present the working tree may hold uncommitted or + # unpushed local work, so refuse to delete it and surface an error + # asking the user to repair/remove it manually rather than silently + # losing their changes. + if not (clone_dir / '.git').exists(): + shutil.rmtree(clone_dir) + else: + raise PluginError( + f"Existing clone '{clone_dir}' has a .git but is missing " + f"registry.yml; refusing to delete it to avoid losing local " + f"changes. Restore registry.yml (e.g. 'git checkout -- " + f"registry.yml') or remove the directory manually, then retry." + ) dir_name = _url_to_repos_dirname(repo.url) repos_dir = registry.get_repos_dir() diff --git a/lib/devbase/plugin/registry.py b/lib/devbase/plugin/registry.py index f3cc58d..61f2fad 100644 --- a/lib/devbase/plugin/registry.py +++ b/lib/devbase/plugin/registry.py @@ -72,15 +72,22 @@ def add_many(self, plugins: list[InstalledPlugin]) -> None: Saving plugins.yml once for a batch avoids the repeated read+atomic rewrite that calling add() per plugin would incur (e.g. during - migration of many legacy installs).""" + migration of many legacy installs). + + Duplicate names within `plugins` are de-duplicated (last one wins) so a + caller passing the same plugin twice can't leave two conflicting entries + in plugins.yml.""" if not plugins: return + # Keep only the last entry per name; dict preserves insertion order so + # the surviving entries stay in the order they were last supplied. + unique = list({p.name: p for p in plugins}.values()) data = self._load() - names = {p.name for p in plugins} + names = {p.name for p in unique} data['installed_plugins'] = [ p for p in data['installed_plugins'] if p['name'] not in names ] - data['installed_plugins'].extend(p.to_dict() for p in plugins) + data['installed_plugins'].extend(p.to_dict() for p in unique) self._save(data) def remove(self, name: str) -> bool: diff --git a/tests/plugin/test_migrator.py b/tests/plugin/test_migrator.py index a3a8557..fddafa3 100644 --- a/tests/plugin/test_migrator.py +++ b/tests/plugin/test_migrator.py @@ -8,6 +8,7 @@ import yaml import pytest +from devbase.errors import PluginError from devbase.plugin.models import ( AvailablePlugin, InstalledPlugin, @@ -653,3 +654,116 @@ def test_update_triggers_migration_of_legacy(self, registry, devbase_root): # only auto-migration removes the stale plugins/ copy; a plain pull # would rewrite the path but leave the old copy on disk. assert not (devbase_root / "plugins" / "adminer").exists() + + +class TestAddManyDedup: + def test_duplicate_names_keep_last(self, registry): + # add_many is a public batch API; passing the same name twice must not + # leave two conflicting entries in plugins.yml — the last wins. + first = _installed("adminer", "repos/x/adminer") + second = InstalledPlugin( + name="adminer", version="2.0.0", + source="https://github.com/testorg/testrepo.git", + installed_at="2026-02-02T00:00:00+00:00", + path="repos/y/adminer", linked=False, + ) + registry.add_many([first, second]) + installed = registry.list_installed() + assert [p.name for p in installed] == ["adminer"] + # Last entry won. + assert registry.get("adminer").path == "repos/y/adminer" + assert registry.get("adminer").version == "2.0.0" + + def test_duplicate_does_not_duplicate_existing(self, registry): + # An existing entry replaced by a batch containing duplicates of that + # name still results in exactly one row. + registry.add(_installed("adminer", "plugins/adminer")) + registry.add_many([ + _installed("adminer", "repos/a/adminer"), + _installed("adminer", "repos/b/adminer"), + ]) + installed = [p for p in registry.list_installed() if p.name == "adminer"] + assert len(installed) == 1 + assert installed[0].path == "repos/b/adminer" + + +class TestFilesEqualExecBitOnly: + """_files_equal should only care about exec bits, not full perms.""" + + def test_rw_perm_diff_does_not_differ(self, tmp_path): + # Identical bytes, both non-executable, but differing read/write bits + # (e.g. from a different umask) must NOT count as divergence. + a, b = tmp_path / "a", tmp_path / "b" + a.mkdir(); b.mkdir() + (a / "f.txt").write_text("same\n") + (b / "f.txt").write_text("same\n") + (a / "f.txt").chmod(0o644) + (b / "f.txt").chmod(0o640) + assert _dirs_differ(a, b) is False + + def test_exec_bit_diff_still_differs(self, tmp_path): + # Functionally meaningful exec-bit change is still detected. + a, b = tmp_path / "a", tmp_path / "b" + a.mkdir(); b.mkdir() + (a / "f.sh").write_text("#!/bin/sh\n") + (b / "f.sh").write_text("#!/bin/sh\n") + (a / "f.sh").chmod(0o755) + (b / "f.sh").chmod(0o644) + assert _dirs_differ(a, b) is True + + +class TestEnsureRepoClonedProtectsGit: + """A recorded local_path whose dir keeps .git must not be deleted.""" + + def test_local_path_with_git_missing_registry_is_not_deleted( + self, registry, devbase_root, + ): + # local_path recorded; clone has .git but registry.yml is gone. The dir + # may hold uncommitted/unpushed local work, so migration must refuse to + # rmtree it and raise instead of silently destroying it. + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _register_repo(registry, plugins) # local_path = repos/ + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + clone = devbase_root / "repos" / DIRNAME + clone.mkdir(parents=True) + (clone / ".git").mkdir() + (clone / "local-work.txt").write_text("uncommitted\n") + # No registry.yml -> _clone_is_healthy is False. + + def fake_clone(url, dest, **kwargs): # pragma: no cover - must not run + raise AssertionError("re-clone must not happen when .git is present") + + with patch("devbase.plugin.installer.git_clone", side_effect=fake_clone): + result = migrate(registry) + + # The plugin was skipped (not migrated) and the dir survives intact. + assert "adminer" in result.skipped + assert (clone / ".git").is_dir() + assert (clone / "local-work.txt").read_text() == "uncommitted\n" + # registry entry still points at the legacy path so it retries later. + assert registry.get("adminer").path == "plugins/adminer" + + def test_local_path_without_git_is_still_recloned(self, registry, devbase_root): + # Sanity: when .git is gone the dir is genuinely broken and re-cloning + # (the existing behaviour) still applies. + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + broken = devbase_root / "repos" / DIRNAME + broken.mkdir(parents=True) + (broken / "leftover.txt").write_text("broken\n") + + def fake_clone(url, dest, **kwargs): + _make_repo_clone(dest.parent.parent, plugins) + + with patch( + "devbase.plugin.installer.git_clone", side_effect=fake_clone, + ) as mock: + result = migrate(registry) + + mock.assert_called_once() + assert result.migrated == ["adminer"] + assert (devbase_root / "repos" / DIRNAME / ".git").exists() + assert not (devbase_root / "repos" / DIRNAME / "leftover.txt").exists() From f1854aa729bcc2de5bec37e81703f481f5dd0650 Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Thu, 28 May 2026 11:55:47 +0000 Subject: [PATCH 7/9] =?UTF-8?q?fix(plugin):=20migration=20=E3=81=AE=20deri?= =?UTF-8?q?ved=20clone=20=E4=BF=9D=E5=85=A8=20/=20registry=20=E5=85=88?= =?UTF-8?q?=E8=A1=8C=E6=B0=B8=E7=B6=9A=E5=8C=96=20/=20=E7=89=B9=E6=AE=8A?= =?UTF-8?q?=E3=83=95=E3=82=A1=E3=82=A4=E3=83=AB=E5=B7=AE=E5=88=86=E6=A4=9C?= =?UTF-8?q?=E7=9F=A5=20(PR2=20round5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cross-review round 5 で指摘された 4 件に対応: - derived clone 経路 (.git 保護): repo.local_path 未設定でも repos/ に .git 付き既存 clone があり registry.yml だけ欠ける場合、無条件 rmtree で 未コミット/未 push のローカル変更を失っていた。_reclaim_or_protect_existing を新設し local_path 経路と同じく .git 有りは削除せず PluginError で復旧案内 する (freshly clone した分のみ破棄)。 - registry 先行永続化: 旧 plugins/ コピーの削除/.bak 退避 → add_many の順序を 逆転し、検証済み path rewrite を破壊的 fs 操作の前に 1 回保存する二相構成へ。 保存失敗時はコピー無傷で abort (次回 retry 可能)、phase2 の retire 失敗は registry が既に有効な repos/ clone を指すため lingering copy として _cleanup_plugins_dir が surface する (silent data loss を排除)。 - clone_dir がファイル/symlink で squat: clone_dir.is_dir() のみでは git_clone が失敗するため、ファイル/symlink は unlink して再 clone (git tree を持たない ため損失なし)。 - _entry_kind == 'other' (socket/pipe/device): 内容比較できず identical を 証明できないため diverged 扱いとし .bak 保全に倒す。 migrator テスト 5 件追加 (derived .git 保護 / clone_dir ファイル squat / registry 保存失敗でコピー無傷 / retire は保存後 / fifo は差分扱い)。 全 275 件 pass。ruff (E9,F63,F7,F82) / compileall pass。 Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/devbase/plugin/migrator.py | 169 ++++++++++++++++++++++----------- tests/plugin/test_migrator.py | 131 +++++++++++++++++++++++++ 2 files changed, 246 insertions(+), 54 deletions(-) diff --git a/lib/devbase/plugin/migrator.py b/lib/devbase/plugin/migrator.py index 5907af3..96bada0 100644 --- a/lib/devbase/plugin/migrator.py +++ b/lib/devbase/plugin/migrator.py @@ -148,6 +148,12 @@ def _entries(base: Path) -> set[Path]: elif kind == 'file': if not _files_equal(fa, fb): return True + elif kind == 'other': + # A socket / pipe / device the copy holds can't be content-compared, + # so it can't be proven identical — treat it as divergence and fall + # back to preserving the copy (.bak) rather than risk deleting data + # we couldn't inspect. + return True return False @@ -165,6 +171,46 @@ def _clone_is_healthy(clone_dir: Path) -> bool: ) +def _reclaim_or_protect_existing(clone_dir: Path) -> None: + """Clear a reusable leftover at clone_dir, or protect a .git-bearing tree. + + Called before (re-)cloning into clone_dir. Three cases: + + - clone_dir is a symlink (broken, to a file, or even to a dir) -> remove + the link; it is never a real persistent clone and git_clone would fail. + - clone_dir does not exist -> nothing to do. + - clone_dir exists but is not a directory (a stray file squatting on the + path) -> remove it so git_clone can create the directory; a regular file + cannot hold a git working tree, so nothing is lost. + - clone_dir is a directory without .git -> a broken/partial clone that can + never be pulled; remove it so a fresh clone repairs it. + - clone_dir is a directory *with* .git -> it may hold uncommitted or + unpushed local work, so refuse to delete it and raise asking the user to + repair/remove it manually rather than silently destroying their changes. + """ + # Check the symlink first: is_dir()/exists() both follow symlinks, so a + # symlink-to-dir would otherwise slip through as a "directory". + if clone_dir.is_symlink(): + clone_dir.unlink() + return + if not clone_dir.exists(): + return + if not clone_dir.is_dir(): + # A regular file is squatting on the path; git_clone would fail. It can + # hold no git working tree, so removing it loses nothing. + clone_dir.unlink() + return + if not (clone_dir / '.git').exists(): + shutil.rmtree(clone_dir) + return + raise PluginError( + f"Existing clone '{clone_dir}' has a .git but is missing " + f"registry.yml; refusing to delete it to avoid losing local " + f"changes. Restore registry.yml (e.g. 'git checkout -- " + f"registry.yml') or remove the directory manually, then retry." + ) + + def _ensure_repo_cloned( registry: PluginRegistry, repo: RegisteredRepository, ) -> tuple[Path, RegisteredRepository]: @@ -182,39 +228,27 @@ def _ensure_repo_cloned( clone_dir = registry.devbase_root / repo.local_path if _clone_is_healthy(clone_dir): return clone_dir, repo - if clone_dir.is_dir(): - # The dir exists but isn't fully healthy. Only destroy it when its - # .git is gone (an un-pullable tree that re-cloning must repair). - # If .git is still present the working tree may hold uncommitted or - # unpushed local work, so refuse to delete it and surface an error - # asking the user to repair/remove it manually rather than silently - # losing their changes. - if not (clone_dir / '.git').exists(): - shutil.rmtree(clone_dir) - else: - raise PluginError( - f"Existing clone '{clone_dir}' has a .git but is missing " - f"registry.yml; refusing to delete it to avoid losing local " - f"changes. Restore registry.yml (e.g. 'git checkout -- " - f"registry.yml') or remove the directory manually, then retry." - ) + _reclaim_or_protect_existing(clone_dir) dir_name = _url_to_repos_dirname(repo.url) repos_dir = registry.get_repos_dir() repos_dir.mkdir(exist_ok=True) clone_dir = repos_dir / dir_name - # A leftover directory from a previously interrupted clone (e.g. disk full - # or network drop) would otherwise be reused forever — re-cloning is - # skipped while parse_registry_yml() keeps failing, so migration can never - # self-heal. Treat a dir that is not a valid clone (missing .git) as - # broken and remove it so the clone below re-creates it cleanly. - if clone_dir.is_dir() and not (clone_dir / '.git').exists(): - shutil.rmtree(clone_dir) + # A leftover from a previously interrupted clone (e.g. disk full or network + # drop) would otherwise be reused forever — re-cloning is skipped while + # parse_registry_yml() keeps failing, so migration can never self-heal. + # Remove a leftover that is *not* a valid clone (missing .git, or a stray + # non-directory squatting on the path) so the clone below re-creates it + # cleanly; but a dir that still has .git may hold uncommitted/unpushed work + # and is protected (raises) rather than destroyed. + _reclaim_or_protect_existing(clone_dir) + freshly_cloned = False if not clone_dir.is_dir(): try: git_clone(repo.url, clone_dir, shallow=False) + freshly_cloned = True except Exception: # Drop a partial clone so the next run starts from a clean slate. if clone_dir.is_dir(): @@ -223,11 +257,20 @@ def _ensure_repo_cloned( reg_info = parse_registry_yml(clone_dir) if not reg_info: - # The clone exists but is missing/invalid registry.yml; discard it so - # a subsequent migration attempt re-clones instead of looping here. - shutil.rmtree(clone_dir) + # registry.yml is missing/invalid. Only discard a clone we just made + # (guaranteed to hold no local work); an existing .git-bearing dir is + # protected so we never delete uncommitted/unpushed changes — surface a + # recoverable error instead, mirroring the local_path branch. + if freshly_cloned: + shutil.rmtree(clone_dir) + raise PluginError( + f"No registry.yml found in cloned repository '{repo.name}'." + ) raise PluginError( - f"No registry.yml found in cloned repository '{repo.name}'." + f"Existing clone '{clone_dir}' has a .git but is missing " + f"registry.yml; refusing to delete it to avoid losing local " + f"changes. Restore registry.yml (e.g. 'git checkout -- " + f"registry.yml') or remove the directory manually, then retry." ) local_path = f"repos/{dir_name}" @@ -304,12 +347,24 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu if not legacy: return result - # Accumulate path rewrites and persist them in a single plugins.yml save - # after the loop instead of one atomic rewrite per plugin. Each plugin's - # filesystem move and its registry entry are built inside the same try - # block, so a failure leaves that plugin out of `pending` (still legacy -> - # retried next run) while completed ones are still committed together. + # Two-phase migration so a registry-save failure can never leave a copy + # deleted while plugins.yml still points at the stale plugins/ path: + # + # Phase 1 (no destructive fs ops): validate the repo/clone/entry and + # decide each copy's fate (delete vs preserve as .bak), collecting the + # repos/ path rewrites in `pending` and the retire actions in `retire`. + # Persist: write every rewrite in a single plugins.yml save. + # Phase 2 (destructive): only after plugins.yml is durably at repos/ do we + # delete/rename the old copies. + # + # Ordering rationale: if the save raises, no copy has been touched yet, so + # the registry stays legacy and the next run retries cleanly with the copies + # intact (recoverable). Conversely the validated repos/ clone is known good + # before we commit, so committing the rewrite first cannot strand a plugin + # on a missing tree; a stray copy left by a phase-2 hiccup is merely surfaced + # by _cleanup_plugins_dir, never silent data loss. pending: list[InstalledPlugin] = [] + retire: list[tuple[str, Path, Path]] = [] # (plugin_name, old_dir, repo_dir) for plugin in legacy: try: @@ -349,28 +404,7 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu info = load_plugin_info(repo_plugin_dir) version = info.version if info else plugin.version - # Retire the old plugins/ copy FIRST, then update the registry. - # Updating plugins.yml before the filesystem move means a failure - # here would leave registry at repos/ (no longer "legacy", so never - # retried) while the stale copy lingers — so the path rewrite must - # only be committed once the copy has been removed or preserved. old_dir = registry.devbase_root / plugin.path - if old_dir.is_dir() and not old_dir.is_symlink(): - if _dirs_differ(old_dir, repo_plugin_dir): - bak = _unique_bak_path(old_dir) - old_dir.rename(bak) - result.preserved.append(plugin.name) - logger.warning( - "Plugin '%s' had local changes — preserved at %s " - "(reconcile manually, then remove)", - plugin.name, bak, - ) - else: - shutil.rmtree(old_dir) - result.migrated.append(plugin.name) - else: - result.migrated.append(plugin.name) - pending.append(InstalledPlugin( name=plugin.name, version=version, @@ -379,13 +413,40 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu path=rel_path, linked=False, )) + retire.append((plugin.name, old_dir, repo_plugin_dir)) except Exception as e: result.skipped.append(plugin.name) result.errors.append(f"{plugin.name}: {e}") - # Single save for every plugin whose copy was successfully retired. + # Persist every validated path rewrite in a single save BEFORE retiring any + # copy. A failure here aborts with the copies untouched (recoverable). registry.add_many(pending) + # Now that plugins.yml durably points at repos/, retire the old copies. + for name, old_dir, repo_plugin_dir in retire: + try: + if old_dir.is_dir() and not old_dir.is_symlink(): + if _dirs_differ(old_dir, repo_plugin_dir): + bak = _unique_bak_path(old_dir) + old_dir.rename(bak) + result.preserved.append(name) + logger.warning( + "Plugin '%s' had local changes — preserved at %s " + "(reconcile manually, then remove)", + name, bak, + ) + else: + shutil.rmtree(old_dir) + result.migrated.append(name) + else: + result.migrated.append(name) + except Exception as e: + # Registry is already at repos/ (valid clone), so the plugin works; + # a copy we failed to retire just lingers under plugins/ and is + # surfaced by _cleanup_plugins_dir rather than lost. + result.migrated.append(name) + result.errors.append(f"{name}: copy not retired: {e}") + if run_sync: sync_projects(registry) diff --git a/tests/plugin/test_migrator.py b/tests/plugin/test_migrator.py index fddafa3..715f694 100644 --- a/tests/plugin/test_migrator.py +++ b/tests/plugin/test_migrator.py @@ -767,3 +767,134 @@ def fake_clone(url, dest, **kwargs): assert result.migrated == ["adminer"] assert (devbase_root / "repos" / DIRNAME / ".git").exists() assert not (devbase_root / "repos" / DIRNAME / "leftover.txt").exists() + + def test_derived_path_with_git_missing_registry_is_not_deleted( + self, registry, devbase_root, + ): + # local_path is NOT recorded (pre-persistent-clone registration) but a + # repos/ clone already exists with .git and only registry.yml + # missing. It may hold uncommitted/unpushed local work, so the derived + # clone path must refuse to rmtree it just as the local_path branch does. + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _register_repo(registry, plugins, local_path=None) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + clone = devbase_root / "repos" / DIRNAME + clone.mkdir(parents=True) + (clone / ".git").mkdir() + (clone / "local-work.txt").write_text("uncommitted\n") + # No registry.yml -> parse_registry_yml fails on the existing dir. + + def fake_clone(url, dest, **kwargs): # pragma: no cover - must not run + raise AssertionError("re-clone must not happen when .git is present") + + with patch("devbase.plugin.installer.git_clone", side_effect=fake_clone): + result = migrate(registry) + + assert "adminer" in result.skipped + assert (clone / ".git").is_dir() + assert (clone / "local-work.txt").read_text() == "uncommitted\n" + # registry entry still legacy so a later run can retry. + assert registry.get("adminer").path == "plugins/adminer" + + def test_clone_dir_existing_as_file_is_replaced(self, registry, devbase_root): + # repos/ is squatted on by a regular *file* (not a directory). + # git_clone would fail; the file holds no git tree so it is removed and + # a fresh clone created in its place. + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _register_repo(registry, plugins, local_path=None) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + repos_dir = devbase_root / "repos" + repos_dir.mkdir(parents=True) + stray = repos_dir / DIRNAME + stray.write_text("not a directory\n") + assert stray.is_file() + + def fake_clone(url, dest, **kwargs): + _make_repo_clone(dest.parent.parent, plugins) + + with patch( + "devbase.plugin.installer.git_clone", side_effect=fake_clone, + ) as mock: + result = migrate(registry) + + mock.assert_called_once() + assert result.migrated == ["adminer"] + assert (devbase_root / "repos" / DIRNAME).is_dir() + assert (devbase_root / "repos" / DIRNAME / ".git").exists() + + +class TestMigratePersistsRegistryBeforeRetiringCopy: + """A registry-save failure must not leave a copy deleted with a stale path. + + Round 3 batched the path rewrites into a single add_many AFTER retiring the + copies; if that save raised, the copies were already gone/renamed while + plugins.yml still pointed at plugins/. migrate() now saves the rewrites + BEFORE any destructive filesystem op, so a save failure aborts with every + copy intact. + """ + + def test_save_failure_leaves_copy_intact(self, registry, devbase_root): + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + + # add_many blows up exactly when migrate() tries to persist the rewrite. + with patch.object( + PluginRegistry, "add_many", side_effect=OSError("disk full"), + ): + with pytest.raises(OSError): + migrate(registry) + + # The copy was NOT deleted or renamed to .bak: it is still right where + # it was, so the next run can retry cleanly. + assert (devbase_root / "plugins" / "adminer" / "plugin.yml").is_file() + assert not (devbase_root / "plugins" / "adminer.bak").exists() + + def test_copy_retired_only_after_registry_persisted(self, registry, devbase_root): + # The copy delete/rename must happen strictly after add_many returns. + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _make_repo_clone(devbase_root, plugins) + _register_repo(registry, plugins) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + + copy = devbase_root / "plugins" / "adminer" + orig_add_many = PluginRegistry.add_many + + def spy_add_many(self, plugins_arg): + # At save time the copy must still exist (not yet retired). + assert copy.is_dir(), "copy retired before registry was persisted" + return orig_add_many(self, plugins_arg) + + with patch.object(PluginRegistry, "add_many", autospec=True, + side_effect=spy_add_many): + result = migrate(registry) + + assert result.migrated == ["adminer"] + # After a successful save the clean copy is gone. + assert not copy.exists() + assert registry.get("adminer").path == f"repos/{DIRNAME}/adminer" + + +class TestDirsDifferOtherEntryKind: + """An entry of kind 'other' (fifo/socket/device) can't be content-compared + and must be treated as divergence so the copy is preserved, not deleted.""" + + def test_fifo_in_copy_differs(self, tmp_path): + import os + a, b = tmp_path / "a", tmp_path / "b" + a.mkdir(); b.mkdir() + (a / "plugin.yml").write_text("name: x\n") + (b / "plugin.yml").write_text("name: x\n") + try: + os.mkfifo(a / "pipe") + os.mkfifo(b / "pipe") + except (AttributeError, NotImplementedError, OSError): + pytest.skip("mkfifo not supported on this platform") + # Both sides hold a fifo at the same path; it can't be proven identical + # so deleting the copy is unsafe -> divergence. + assert _dirs_differ(a, b) is True From 71d56d262995ea976563c52425151d5b73eec621 Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Thu, 28 May 2026 12:12:18 +0000 Subject: [PATCH 8/9] =?UTF-8?q?fix(plugin):=20derived=20clone=20=E7=B5=8C?= =?UTF-8?q?=E8=B7=AF=E3=81=A7=E5=81=A5=E5=85=A8=20clone=20=E3=82=92=20reus?= =?UTF-8?q?e=20=E3=81=99=E3=82=8B=20(round=206)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit round 5 で derived 経路 (local_path 未設定) の `.git` 付き既存 dir を 無条件に保護 (PluginError) していたが過剰だった。`repos/` に .git + registry.yml が両方そろった健全 clone が残っている場合は PluginError で migration を skip せず、そのまま reuse して local_path を 永続化するよう修正。 - 健全 clone (.git + registry.yml) → reuse + local_path 永続化 - .git ありだが unhealthy (registry.yml 欠落) → 従来どおり保護 (PluginError) - .git 無し / file・symlink squat → reclaim して再 clone local_path 経路と derived 経路で挙動を揃え、fresh clone 後と healthy reuse 後の local_path 永続化を `_persist_repo_local_path` に共通化した。 健全 derived clone が reuse され migration が skip されないことを検証する テスト `test_derived_path_with_healthy_clone_is_reused` を追加。 Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/devbase/plugin/migrator.py | 51 ++++++++++++++++++++++++++-------- tests/plugin/test_migrator.py | 32 +++++++++++++++++++++ 2 files changed, 71 insertions(+), 12 deletions(-) diff --git a/lib/devbase/plugin/migrator.py b/lib/devbase/plugin/migrator.py index 96bada0..cbe6162 100644 --- a/lib/devbase/plugin/migrator.py +++ b/lib/devbase/plugin/migrator.py @@ -211,6 +211,33 @@ def _reclaim_or_protect_existing(clone_dir: Path) -> None: ) +def _persist_repo_local_path( + registry: PluginRegistry, repo: RegisteredRepository, dir_name: str, +) -> RegisteredRepository: + """Record local_path = repos/ + a refreshed plugin list for repo. + + Used after a fresh clone *and* when reusing a healthy clone left by an + earlier run (a pre-persistent-clone registration with no local_path), so the + plugins.yml entry is repaired identically in both cases and future runs take + the local_path fast path. + """ + from .installer import parse_registry_yml + + local_path = f"repos/{dir_name}" + clone_dir = registry.devbase_root / local_path + reg_info = parse_registry_yml(clone_dir) + plugins = [ + AvailablePlugin(name=e.name, description=e.description, path=e.path) + for e in reg_info.plugins + ] if reg_info else list(repo.plugins) + updated = RegisteredRepository( + name=repo.name, url=repo.url, added_at=repo.added_at, + local_path=local_path, plugins=plugins, + ) + registry.add_repository(updated) + return updated + + def _ensure_repo_cloned( registry: PluginRegistry, repo: RegisteredRepository, ) -> tuple[Path, RegisteredRepository]: @@ -219,7 +246,9 @@ def _ensure_repo_cloned( If the repo was registered before persistent-clone support (no local_path), the clone dir is missing, or the existing clone is broken (missing .git / registry.yml), perform a full clone and persist local_path + a refreshed - plugin list to plugins.yml. + plugin list to plugins.yml. A healthy repos/ clone left by an + earlier run is reused (and its missing local_path persisted) rather than + re-cloned or protected. """ from .installer import git_clone, parse_registry_yml from .repo_manager import _url_to_repos_dirname @@ -235,6 +264,13 @@ def _ensure_repo_cloned( repos_dir.mkdir(exist_ok=True) clone_dir = repos_dir / dir_name + # A pre-persistent-clone registration (no local_path) may already have a + # healthy repos/ clone from an earlier run; reuse it instead of + # protecting (raising) on its .git, just like the local_path branch above — + # only persist the missing local_path so future runs take the fast path. + if _clone_is_healthy(clone_dir): + return clone_dir, _persist_repo_local_path(registry, repo, dir_name) + # A leftover from a previously interrupted clone (e.g. disk full or network # drop) would otherwise be reused forever — re-cloning is skipped while # parse_registry_yml() keeps failing, so migration can never self-heal. @@ -273,17 +309,8 @@ def _ensure_repo_cloned( f"registry.yml') or remove the directory manually, then retry." ) - local_path = f"repos/{dir_name}" - updated = RegisteredRepository( - name=repo.name, url=repo.url, added_at=repo.added_at, - local_path=local_path, - plugins=[ - AvailablePlugin(name=e.name, description=e.description, path=e.path) - for e in reg_info.plugins - ], - ) - registry.add_repository(updated) - logger.info("Repository '%s' cloned to %s", repo.name, local_path) + updated = _persist_repo_local_path(registry, repo, dir_name) + logger.info("Repository '%s' cloned to %s", repo.name, updated.local_path) return clone_dir, updated diff --git a/tests/plugin/test_migrator.py b/tests/plugin/test_migrator.py index 715f694..54ef693 100644 --- a/tests/plugin/test_migrator.py +++ b/tests/plugin/test_migrator.py @@ -797,6 +797,38 @@ def fake_clone(url, dest, **kwargs): # pragma: no cover - must not run # registry entry still legacy so a later run can retry. assert registry.get("adminer").path == "plugins/adminer" + def test_derived_path_with_healthy_clone_is_reused( + self, registry, devbase_root, + ): + # local_path is NOT recorded (pre-persistent-clone registration) but a + # *healthy* repos/ clone (.git AND registry.yml) was left by an + # earlier run. It must be reused — migration proceeds, the missing + # local_path is persisted — not protected (skipped) on its .git nor + # re-cloned. + plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] + _register_repo(registry, plugins, local_path=None) + _make_legacy_copy(devbase_root, "adminer", plugins[0]) + registry.add(_installed("adminer", "plugins/adminer")) + # Healthy clone already present (sentinel proves it is reused, not wiped). + clone = _make_repo_clone(devbase_root, plugins) + (clone / ".git" / "sentinel").write_text("kept\n") + + def fake_clone(url, dest, **kwargs): # pragma: no cover - must not run + raise AssertionError("must reuse a healthy clone, never re-clone") + + with patch("devbase.plugin.installer.git_clone", side_effect=fake_clone): + result = migrate(registry) + + # Migration succeeded (NOT skipped) and the existing clone survived. + assert result.migrated == ["adminer"] + assert "adminer" not in result.skipped + assert (clone / ".git" / "sentinel").read_text() == "kept\n" + # The previously-missing local_path was persisted, and the plugin now + # points at the reused repos/ clone. + repo = registry.get_repository_by_url(URL) + assert repo.local_path == f"repos/{DIRNAME}" + assert registry.get("adminer").path == f"repos/{DIRNAME}/adminer" + def test_clone_dir_existing_as_file_is_replaced(self, registry, devbase_root): # repos/ is squatted on by a regular *file* (not a directory). # git_clone would fail; the file holds no git tree so it is removed and From 0c7549bcc9a46849e4b5ba38120103c12dbc6644 Mon Sep 17 00:00:00 2001 From: "takemi.ohama" Date: Thu, 28 May 2026 15:09:16 +0000 Subject: [PATCH 9/9] =?UTF-8?q?perf(plugin):=20migration=20=E3=81=AE=20clo?= =?UTF-8?q?ne=20=E6=B0=B8=E7=B6=9A=E5=8C=96=E3=82=92=E5=8D=98=E4=B8=80?= =?UTF-8?q?=E4=BF=9D=E5=AD=98=E3=81=AB=E9=9B=86=E7=B4=84=20(round=207)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _ensure_repo_cloned が clone のたびに add_repository で plugins.yml を 保存していたため、多数リポジトリ移行時に保存回数が repo 数に比例していた。 clone 済み repo 行を pending_repos に貯め、path rewrite と合わせて Phase2 (破壊的 cleanup) の直前に save_migration で 1 回だけ保存するよう変更。 二相アトミシティは維持: 旧 copy 削除より前に registry が必ず flush 済みで あること (clone を指す local_path / plugin path の両方) を不変条件として 保持。save_migration は repos + plugins を単一 load+save で upsert する。 Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/devbase/plugin/migrator.py | 59 +++++++++---- lib/devbase/plugin/registry.py | 71 +++++++++++++--- tests/plugin/test_migrator.py | 146 ++++++++++++++++++++++++++++++--- 3 files changed, 234 insertions(+), 42 deletions(-) diff --git a/lib/devbase/plugin/migrator.py b/lib/devbase/plugin/migrator.py index cbe6162..bba767b 100644 --- a/lib/devbase/plugin/migrator.py +++ b/lib/devbase/plugin/migrator.py @@ -211,15 +211,20 @@ def _reclaim_or_protect_existing(clone_dir: Path) -> None: ) -def _persist_repo_local_path( +def _build_persisted_repo( registry: PluginRegistry, repo: RegisteredRepository, dir_name: str, ) -> RegisteredRepository: - """Record local_path = repos/ + a refreshed plugin list for repo. + """Build a repo row with local_path = repos/ + a refreshed plugin + list, WITHOUT saving. Used after a fresh clone *and* when reusing a healthy clone left by an earlier run (a pre-persistent-clone registration with no local_path), so the plugins.yml entry is repaired identically in both cases and future runs take the local_path fast path. + + The caller is responsible for persisting the returned row: during migration + every repo update is accumulated and flushed in a single plugins.yml save + (see migrate()), so this no longer writes per repo. """ from .installer import parse_registry_yml @@ -230,25 +235,31 @@ def _persist_repo_local_path( AvailablePlugin(name=e.name, description=e.description, path=e.path) for e in reg_info.plugins ] if reg_info else list(repo.plugins) - updated = RegisteredRepository( + return RegisteredRepository( name=repo.name, url=repo.url, added_at=repo.added_at, local_path=local_path, plugins=plugins, ) - registry.add_repository(updated) - return updated def _ensure_repo_cloned( - registry: PluginRegistry, repo: RegisteredRepository, + registry: PluginRegistry, + repo: RegisteredRepository, + pending_repos: list[RegisteredRepository], ) -> tuple[Path, RegisteredRepository]: """Return the repos/ clone dir for a repo, cloning it if necessary. If the repo was registered before persistent-clone support (no local_path), the clone dir is missing, or the existing clone is broken (missing .git / - registry.yml), perform a full clone and persist local_path + a refreshed - plugin list to plugins.yml. A healthy repos/ clone left by an - earlier run is reused (and its missing local_path persisted) rather than + registry.yml), perform a full clone and stage local_path + a refreshed + plugin list for persistence. A healthy repos/ clone left by an + earlier run is reused (and its missing local_path staged) rather than re-cloned or protected. + + Any repo row that needs (re)persisting is appended to `pending_repos` + instead of being saved here; migrate() flushes them in a single + plugins.yml save before any destructive cleanup, so the registry still + durably points at the clone before old copies are retired, but the save + count stays O(1) rather than one save per cloned repo. """ from .installer import git_clone, parse_registry_yml from .repo_manager import _url_to_repos_dirname @@ -267,9 +278,11 @@ def _ensure_repo_cloned( # A pre-persistent-clone registration (no local_path) may already have a # healthy repos/ clone from an earlier run; reuse it instead of # protecting (raising) on its .git, just like the local_path branch above — - # only persist the missing local_path so future runs take the fast path. + # only stage the missing local_path so future runs take the fast path. if _clone_is_healthy(clone_dir): - return clone_dir, _persist_repo_local_path(registry, repo, dir_name) + updated = _build_persisted_repo(registry, repo, dir_name) + pending_repos.append(updated) + return clone_dir, updated # A leftover from a previously interrupted clone (e.g. disk full or network # drop) would otherwise be reused forever — re-cloning is skipped while @@ -309,7 +322,8 @@ def _ensure_repo_cloned( f"registry.yml') or remove the directory manually, then retry." ) - updated = _persist_repo_local_path(registry, repo, dir_name) + updated = _build_persisted_repo(registry, repo, dir_name) + pending_repos.append(updated) logger.info("Repository '%s' cloned to %s", repo.name, updated.local_path) return clone_dir, updated @@ -379,8 +393,12 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu # # Phase 1 (no destructive fs ops): validate the repo/clone/entry and # decide each copy's fate (delete vs preserve as .bak), collecting the - # repos/ path rewrites in `pending` and the retire actions in `retire`. - # Persist: write every rewrite in a single plugins.yml save. + # cloned-repo rows in `pending_repos`, the repos/ path rewrites in + # `pending`, and the retire actions in `retire`. Cloning stages the + # repo row in `pending_repos` rather than saving per clone, so the save + # count stays O(1) regardless of how many repos are cloned. + # Persist: write every repo row + path rewrite in a single plugins.yml + # save (save_migration), flushing the staged clones BEFORE any cleanup. # Phase 2 (destructive): only after plugins.yml is durably at repos/ do we # delete/rename the old copies. # @@ -391,6 +409,7 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu # on a missing tree; a stray copy left by a phase-2 hiccup is merely surfaced # by _cleanup_plugins_dir, never silent data loss. pending: list[InstalledPlugin] = [] + pending_repos: list[RegisteredRepository] = [] # cloned-repo rows to persist retire: list[tuple[str, Path, Path]] = [] # (plugin_name, old_dir, repo_dir) for plugin in legacy: @@ -404,7 +423,7 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu ) continue - clone_dir, repo = _ensure_repo_cloned(registry, repo) + clone_dir, repo = _ensure_repo_cloned(registry, repo, pending_repos) reg_info = parse_registry_yml(clone_dir) entry = None @@ -445,9 +464,13 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu result.skipped.append(plugin.name) result.errors.append(f"{plugin.name}: {e}") - # Persist every validated path rewrite in a single save BEFORE retiring any - # copy. A failure here aborts with the copies untouched (recoverable). - registry.add_many(pending) + # Persist every staged cloned-repo row AND validated path rewrite in a + # single save BEFORE retiring any copy. This both (a) keeps the registry + # durably pointing at the repos/ clones before destructive cleanup — the + # two-phase atomicity invariant — and (b) collapses what used to be one save + # per cloned repo plus the path-rewrite save into a single O(1) write. A + # failure here aborts with the copies untouched (recoverable). + registry.save_migration(pending_repos, pending) # Now that plugins.yml durably points at repos/, retire the old copies. for name, old_dir, repo_plugin_dir in retire: diff --git a/lib/devbase/plugin/registry.py b/lib/devbase/plugin/registry.py index 61f2fad..b5eb6ed 100644 --- a/lib/devbase/plugin/registry.py +++ b/lib/devbase/plugin/registry.py @@ -67,27 +67,77 @@ def add(self, plugin: InstalledPlugin) -> None: """Add a plugin to the registry""" self.add_many([plugin]) - def add_many(self, plugins: list[InstalledPlugin]) -> None: - """Add/replace multiple plugins with a single load+save. - - Saving plugins.yml once for a batch avoids the repeated read+atomic - rewrite that calling add() per plugin would incur (e.g. during - migration of many legacy installs). + @staticmethod + def _apply_plugins(data: dict, plugins: list[InstalledPlugin]) -> None: + """Upsert `plugins` into `data['installed_plugins']` (in place). Duplicate names within `plugins` are de-duplicated (last one wins) so a caller passing the same plugin twice can't leave two conflicting entries - in plugins.yml.""" + in plugins.yml. Does not touch disk — callers persist `data` once. + """ if not plugins: return # Keep only the last entry per name; dict preserves insertion order so # the surviving entries stay in the order they were last supplied. unique = list({p.name: p for p in plugins}.values()) - data = self._load() names = {p.name for p in unique} data['installed_plugins'] = [ p for p in data['installed_plugins'] if p['name'] not in names ] data['installed_plugins'].extend(p.to_dict() for p in unique) + + @staticmethod + def _apply_repositories( + data: dict, repos: list[RegisteredRepository], + ) -> None: + """Upsert `repos` into `data['repositories']` (in place). + + Duplicate names (last one wins) are de-duplicated so accumulating the + same repo twice can't leave two conflicting rows. Does not touch disk. + """ + if not repos: + return + unique = list({r.name: r for r in repos}.values()) + names = {r.name for r in unique} + data['repositories'] = [ + r for r in data['repositories'] if r['name'] not in names + ] + data['repositories'].extend(r.to_dict() for r in unique) + + def add_many(self, plugins: list[InstalledPlugin]) -> None: + """Add/replace multiple plugins with a single load+save. + + Saving plugins.yml once for a batch avoids the repeated read+atomic + rewrite that calling add() per plugin would incur (e.g. during + migration of many legacy installs). + + Duplicate names within `plugins` are de-duplicated (last one wins) so a + caller passing the same plugin twice can't leave two conflicting entries + in plugins.yml.""" + if not plugins: + return + data = self._load() + self._apply_plugins(data, plugins) + self._save(data) + + def save_migration( + self, + repositories: list[RegisteredRepository], + plugins: list[InstalledPlugin], + ) -> None: + """Persist repository + plugin updates from a migration in ONE save. + + migrate() clones each legacy repo and rewrites each plugin's path; both + the refreshed repository rows (local_path + plugin list) and the plugin + path rewrites must land durably *before* any old copy is deleted. This + applies both sets of upserts to a single loaded snapshot and writes + plugins.yml exactly once, so the save count stays O(1) regardless of how + many repos were cloned (rather than one save per cloned repo).""" + if not repositories and not plugins: + return + data = self._load() + self._apply_repositories(data, repositories) + self._apply_plugins(data, plugins) self._save(data) def remove(self, name: str) -> bool: @@ -143,10 +193,7 @@ def get_repository_by_url(self, url: str) -> Optional[RegisteredRepository]: def add_repository(self, repo: RegisteredRepository) -> None: """Add or update a repository in the registry""" data = self._load() - data['repositories'] = [ - r for r in data['repositories'] if r['name'] != repo.name - ] - data['repositories'].append(repo.to_dict()) + self._apply_repositories(data, [repo]) self._save(data) def remove_repository(self, name: str) -> bool: diff --git a/tests/plugin/test_migrator.py b/tests/plugin/test_migrator.py index 54ef693..a3fb2c0 100644 --- a/tests/plugin/test_migrator.py +++ b/tests/plugin/test_migrator.py @@ -561,6 +561,85 @@ def test_multiple_plugins_all_persisted_with_single_save(self, registry, devbase for p in plugins: assert registry.get(p["name"]).path == f"repos/{DIRNAME}/{p['name']}" + def test_many_cloned_repos_persist_with_single_save(self, registry, devbase_root): + """O(1) saves even when every repo must be freshly cloned. + + Previously _ensure_repo_cloned saved plugins.yml once per cloned repo + (via add_repository), so migrating N repos cost N repo-saves + 1 + path-rewrite save. migrate() now stages every cloned repo row and + flushes them together with the path rewrites in one save_migration, so + the save count is O(1) regardless of repo count — while still persisting + the clones BEFORE any copy is retired (two-phase atomicity). + """ + from devbase.plugin.repo_manager import _url_to_repos_dirname + + repos = [ + ("https://github.com/testorg/repo-a.git", "alpha"), + ("https://github.com/testorg/repo-b.git", "beta"), + ("https://github.com/testorg/repo-c.git", "gamma"), + ] + dirnames = {url: _url_to_repos_dirname(url) for url, _ in repos} + + for url, pname in repos: + # Register each repo WITHOUT local_path so _ensure_repo_cloned takes + # the fresh-clone path (which used to save per repo). + registry.add_repository(RegisteredRepository( + name=pname, url=url, added_at=registry.now_iso(), local_path="", + plugins=[AvailablePlugin(name=pname, description="", path=pname)], + )) + # Legacy plugins/ copy + installed entry pointing at it. + pdir = devbase_root / "plugins" / pname + pdir.mkdir(parents=True, exist_ok=True) + (pdir / "plugin.yml").write_text(yaml.dump({"name": pname, "version": "1.0.0"})) + (pdir / "projects" / pname).mkdir(parents=True, exist_ok=True) + (pdir / "projects" / pname / "compose.yml").write_text("services: {}\n") + registry.add(InstalledPlugin( + name=pname, version="1.0.0", source=url, + installed_at="2026-01-01T00:00:00+00:00", + path=f"plugins/{pname}", linked=False, + )) + + def fake_clone(url, dest, **kwargs): + # Build a healthy clone identical (byte-for-byte) to the legacy copy + # so it is cleanly retired, proving the registry was persisted first. + pname = dest.name.split("--")[-1].replace("repo-", "") + # Map dest dirname back to the plugin name via the registered repos. + for u, p in repos: + if dest.name == dirnames[u]: + pname = p + break + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir(exist_ok=True) + pdir = dest / pname + pdir.mkdir(parents=True, exist_ok=True) + (pdir / "plugin.yml").write_text(yaml.dump({"name": pname, "version": "1.0.0"})) + (pdir / "projects" / pname).mkdir(parents=True, exist_ok=True) + (pdir / "projects" / pname / "compose.yml").write_text("services: {}\n") + (dest / "registry.yml").write_text(yaml.dump( + {"name": pname, "plugins": [{"name": pname, "path": pname, "description": ""}]} + )) + + with patch("devbase.plugin.installer.git_clone", side_effect=fake_clone): + with patch.object( + PluginRegistry, "_save", autospec=True, + side_effect=PluginRegistry._save, + ) as save_spy: + result = migrate(registry) + + assert sorted(result.migrated) == ["alpha", "beta", "gamma"] + # 3 cloned repos + 3 path rewrites, yet a single plugins.yml write. + assert save_spy.call_count == 1 + for url, pname in repos: + # Each repo row got its local_path persisted (registry durably points + # at the clone)... + repo = registry.get_repository_by_url(url) + assert repo.local_path == f"repos/{dirnames[url]}" + # ...and each plugin path was rewritten to the repos/ clone. + assert registry.get(pname).path == f"repos/{dirnames[url]}/{pname}" + # The legacy copy was retired only because the registry was persisted + # before Phase 2 (the clone is byte-identical, so it is deleted). + assert not (devbase_root / "plugins" / pname).exists() + class TestCmdPluginMigrate: def test_command_runs_migration(self, devbase_root): @@ -687,6 +766,48 @@ def test_duplicate_does_not_duplicate_existing(self, registry): assert installed[0].path == "repos/b/adminer" +class TestSaveMigration: + """save_migration upserts repos + plugins in a single load+save.""" + + def test_repos_and_plugins_applied_in_one_save(self, registry): + repo = RegisteredRepository( + name="testrepo", url=URL, added_at=registry.now_iso(), + local_path=f"repos/{DIRNAME}", + plugins=[AvailablePlugin(name="adminer", description="", path="adminer")], + ) + plugin = _installed("adminer", f"repos/{DIRNAME}/adminer") + + with patch.object( + PluginRegistry, "_save", autospec=True, side_effect=PluginRegistry._save, + ) as save_spy: + registry.save_migration([repo], [plugin]) + + assert save_spy.call_count == 1 + assert registry.get_repository_by_url(URL).local_path == f"repos/{DIRNAME}" + assert registry.get("adminer").path == f"repos/{DIRNAME}/adminer" + + def test_repo_upsert_replaces_existing_row(self, registry): + # A repo registered without local_path is replaced (not duplicated) by + # the migration row carrying local_path. + registry.add_repository(RegisteredRepository( + name="testrepo", url=URL, added_at=registry.now_iso(), local_path="", + )) + updated = RegisteredRepository( + name="testrepo", url=URL, added_at=registry.now_iso(), + local_path=f"repos/{DIRNAME}", + ) + registry.save_migration([updated], []) + + repos = registry.list_repositories() + assert len(repos) == 1 + assert repos[0].local_path == f"repos/{DIRNAME}" + + def test_empty_inputs_do_not_save(self, registry): + with patch.object(PluginRegistry, "_save", autospec=True) as save_spy: + registry.save_migration([], []) + save_spy.assert_not_called() + + class TestFilesEqualExecBitOnly: """_files_equal should only care about exec bits, not full perms.""" @@ -860,11 +981,12 @@ def fake_clone(url, dest, **kwargs): class TestMigratePersistsRegistryBeforeRetiringCopy: """A registry-save failure must not leave a copy deleted with a stale path. - Round 3 batched the path rewrites into a single add_many AFTER retiring the + Round 3 batched the path rewrites into a single save AFTER retiring the copies; if that save raised, the copies were already gone/renamed while - plugins.yml still pointed at plugins/. migrate() now saves the rewrites - BEFORE any destructive filesystem op, so a save failure aborts with every - copy intact. + plugins.yml still pointed at plugins/. migrate() now persists the rewrites + (and any freshly cloned repo rows) via a single save_migration call BEFORE + any destructive filesystem op, so a save failure aborts with every copy + intact. """ def test_save_failure_leaves_copy_intact(self, registry, devbase_root): @@ -874,9 +996,9 @@ def test_save_failure_leaves_copy_intact(self, registry, devbase_root): _make_legacy_copy(devbase_root, "adminer", plugins[0]) registry.add(_installed("adminer", "plugins/adminer")) - # add_many blows up exactly when migrate() tries to persist the rewrite. + # save_migration blows up exactly when migrate() tries to persist. with patch.object( - PluginRegistry, "add_many", side_effect=OSError("disk full"), + PluginRegistry, "save_migration", side_effect=OSError("disk full"), ): with pytest.raises(OSError): migrate(registry) @@ -887,7 +1009,7 @@ def test_save_failure_leaves_copy_intact(self, registry, devbase_root): assert not (devbase_root / "plugins" / "adminer.bak").exists() def test_copy_retired_only_after_registry_persisted(self, registry, devbase_root): - # The copy delete/rename must happen strictly after add_many returns. + # The copy delete/rename must happen strictly after save_migration returns. plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}] _make_repo_clone(devbase_root, plugins) _register_repo(registry, plugins) @@ -895,15 +1017,15 @@ def test_copy_retired_only_after_registry_persisted(self, registry, devbase_root registry.add(_installed("adminer", "plugins/adminer")) copy = devbase_root / "plugins" / "adminer" - orig_add_many = PluginRegistry.add_many + orig_save_migration = PluginRegistry.save_migration - def spy_add_many(self, plugins_arg): + def spy_save_migration(self, repos_arg, plugins_arg): # At save time the copy must still exist (not yet retired). assert copy.is_dir(), "copy retired before registry was persisted" - return orig_add_many(self, plugins_arg) + return orig_save_migration(self, repos_arg, plugins_arg) - with patch.object(PluginRegistry, "add_many", autospec=True, - side_effect=spy_add_many): + with patch.object(PluginRegistry, "save_migration", autospec=True, + side_effect=spy_save_migration): result = migrate(registry) assert result.migrated == ["adminer"]