Skip to content

Commit 65be8ac

Browse files
takemi-ohamaclaude
andcommitted
fix(plugin): repo refresh の壊れた移行を成功扱いにせず移行時の registry.yml 重複パースを抑制
- refresh_repository: _update_repo_plugins が repo_errors を返した場合は warning で握りつぶさず RepositoryError として伝播。pull 後に削除された プラグインの移行失敗を成功扱いにしない (major) - migrate: 同一リポジトリの複数プラグイン移行時、local_path fast path で 返る registry.yml の遅延パースを URL 単位でキャッシュしリポジトリあたり 1 回に抑制 (minor / performance) - 両挙動の回帰テストを追加 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 5a6158a commit 65be8ac

4 files changed

Lines changed: 75 additions & 4 deletions

File tree

lib/devbase/plugin/migrator.py

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,12 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu
434434
repo.url: repo for repo in registry.list_repositories() if repo.url
435435
}
436436

437+
# Cache the lazily-parsed registry.yml per repo URL. On the healthy
438+
# local_path fast path _ensure_repo_cloned returns reg_info=None, so a repo
439+
# with multiple legacy plugins would otherwise re-parse the same registry.yml
440+
# once per plugin here; the cache collapses that to one parse per repo.
441+
reg_info_by_url: dict[str, Optional["RegistryInfo"]] = {}
442+
437443
for plugin in legacy:
438444
try:
439445
repo = repos_by_url.get(plugin.source) if plugin.source else None
@@ -450,10 +456,13 @@ def migrate(registry: PluginRegistry, *, run_sync: bool = True) -> MigrationResu
450456
)
451457

452458
# _ensure_repo_cloned already parsed registry.yml on the clone/reuse
453-
# paths; only the healthy local_path fast path returns None, so parse
454-
# lazily there instead of unconditionally re-reading the same file.
459+
# paths; only the healthy local_path fast path returns None. Parse
460+
# lazily there, but reuse a cached parse for subsequent plugins of
461+
# the same repo instead of re-reading the same file each iteration.
455462
if reg_info is None:
456-
reg_info = parse_registry_yml(clone_dir)
463+
if repo.url not in reg_info_by_url:
464+
reg_info_by_url[repo.url] = parse_registry_yml(clone_dir)
465+
reg_info = reg_info_by_url[repo.url]
457466
entry = None
458467
if reg_info:
459468
entry = next(

lib/devbase/plugin/repo_manager.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -457,8 +457,17 @@ def refresh_repository(
457457
pre_pull_projects=pre_pull_projects,
458458
)
459459
if repo_errors:
460+
# A failed removal-migration leaves the stale plugins/ entry in
461+
# plugins.yml; reporting "refreshed" here would mask that broken
462+
# install state. Surface it as a hard error (mirrors update_plugin,
463+
# which raises on _update_repo_plugins errors) instead of warning and
464+
# exiting 0.
460465
for err in repo_errors:
461-
logger.warning(" %s", err)
466+
logger.error(" %s", err)
467+
raise RepositoryError(
468+
f"Repository '{repo.name}' refresh left plugins in a broken state:\n"
469+
+ "\n".join(f" - {e}" for e in repo_errors)
470+
)
462471
if sync:
463472
sync_projects(registry)
464473

tests/plugin/test_migrator.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,36 @@ def fake_clone(url, dest, **kwargs):
415415
assert registry.get("adminer").path == f"repos/{DIRNAME}/adminer"
416416

417417

418+
def test_registry_yml_parsed_once_for_multiple_plugins_same_repo(
419+
self, registry, devbase_root
420+
):
421+
# Two legacy plugins from the same repo with a healthy local_path clone:
422+
# _ensure_repo_cloned takes the fast path (returns reg_info=None) for
423+
# both, so migrate()'s per-URL reg_info cache must collapse the lazy
424+
# registry.yml parse to one per repo instead of one per plugin.
425+
plugins = [
426+
{"name": "p1", "path": "p1", "projects": ["p1"]},
427+
{"name": "p2", "path": "p2", "projects": ["p2"]},
428+
]
429+
_make_repo_clone(devbase_root, plugins)
430+
_register_repo(registry, plugins)
431+
for p in plugins:
432+
_make_legacy_copy(devbase_root, p["name"], p)
433+
registry.add(_installed(p["name"], f"plugins/{p['name']}"))
434+
435+
import devbase.plugin.installer as installer_mod
436+
real_parse = installer_mod.parse_registry_yml
437+
with patch(
438+
"devbase.plugin.installer.parse_registry_yml",
439+
side_effect=real_parse,
440+
) as spy_parse:
441+
result = migrate(registry)
442+
443+
assert sorted(result.migrated) == ["p1", "p2"]
444+
# Without the cache this would be 2 (one lazy parse per plugin).
445+
assert spy_parse.call_count == 1
446+
447+
418448
class TestMigrateKeepsLinked:
419449
def test_linked_install_keeps_plugins_dir(self, registry, devbase_root):
420450
plugins = [{"name": "adminer", "path": "adminer", "projects": ["adminer"]}]

tests/plugin/test_repos_core.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,29 @@ def test_refresh_warns_removed_installed_plugin(self, registry, devbase_root):
334334
"p1", "testrepo",
335335
)
336336

337+
def test_refresh_raises_when_update_repo_plugins_errors(
338+
self, registry, devbase_root
339+
):
340+
# A failed removal-migration leaves the stale plugins/ entry in
341+
# plugins.yml; refresh must surface that as RepositoryError rather than
342+
# warning-and-exiting-0 (otherwise `devbase plugin repo refresh` reports
343+
# success on a broken install state).
344+
url = "https://github.com/testorg/testrepo.git"
345+
_make_repo_dir(devbase_root, "testorg/testrepo", [
346+
{"name": "p1", "path": "p1"},
347+
])
348+
_register_repo(registry, "testorg/testrepo", url, [
349+
{"name": "p1", "path": "p1"},
350+
])
351+
352+
with patch("devbase.plugin.repo_manager._git_pull"), \
353+
patch(
354+
"devbase.plugin.updater._update_repo_plugins",
355+
return_value=["Migration failed for 'p1'"],
356+
):
357+
with pytest.raises(RepositoryError, match="broken state"):
358+
refresh_repository(registry, "testrepo")
359+
337360

338361
# ── installer.py ────────────────────────────────────────────────
339362

0 commit comments

Comments
 (0)