From b5140e6c60cee55f078b332749461c70e51548ab Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 23 Sep 2026 22:37:24 -0700 Subject: [PATCH] fix: refresh module caches by source identity Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- AGENTS.md | 3 + amplifier_app_cli/utils/module_cache.py | 30 +-- tests/test_module_cache.py | 309 ++++++++++++++++++++++++ 3 files changed, 325 insertions(+), 17 deletions(-) create mode 100644 tests/test_module_cache.py diff --git a/AGENTS.md b/AGENTS.md index bede34cb..63e3a5d8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,6 +81,9 @@ credential-safe, globally unique display labels; malformed app entries render as a generic failed check and never leak their configured payload. Exercise the real Click command in `tests/test_update_reporting.py`; mock only status/apply boundaries so tests never touch a user's caches or installation. +Module cache force-refresh identity is Foundation's exact URL + ref cache key, +never a semantic bundle name or module entry-point ID. Verify it with +`uv run pytest tests/test_module_cache.py`. ## Persisted reminder display diff --git a/amplifier_app_cli/utils/module_cache.py b/amplifier_app_cli/utils/module_cache.py index 8e59b282..84eec55c 100644 --- a/amplifier_app_cli/utils/module_cache.py +++ b/amplifier_app_cli/utils/module_cache.py @@ -471,10 +471,11 @@ async def update_module( ref: str, progress_callback: Callable[[str, str], None] | None = None, ) -> Path: - """Clear cache and immediately re-download a module. + """Force-refresh a module cache entry through Foundation. - Single source of truth for update (clear + re-download). - Uses foundation's SimpleSourceResolver (proper git clone, not uv pip install). + Foundation identifies git cache entries by source URL and ref. Its supported + ``GitSourceHandler.update()`` operation refreshes that exact entry under its + lock, without selecting cache entries from a semantic module or bundle name. Args: url: Git repository URL @@ -482,14 +483,14 @@ async def update_module( progress_callback: Optional callback(module_id, status) for progress Returns: - Path to the newly downloaded module + Path to the refreshed module """ - from amplifier_foundation.sources import SimpleSourceResolver + from amplifier_foundation.paths.resolution import parse_uri + from amplifier_foundation.sources.git import GitSourceHandler repo_name = _extract_repo_name(url) - # Normalize to match how scan_cached_modules() derives module_id from - # pyproject.toml entry points (e.g., "provider-anthropic" not - # "amplifier-module-provider-anthropic") + # The display label remains backward-compatible progress presentation only. + # It is not used to select a cache entry for refresh. if repo_name.startswith("amplifier-module-"): module_id = repo_name[len("amplifier-module-") :] else: @@ -499,20 +500,15 @@ async def update_module( if progress_callback: progress_callback(module_id, "clearing") - # Clear existing cache for this module - clear_module_cache(module_id=module_id) - # Report progress: downloading if progress_callback: progress_callback(module_id, "downloading") - # Build git URI in foundation format (git+url@ref) - uri = f"git+{url}@{ref}" - - # Use foundation's resolver (creates proper .git directory via git clone) cache_dir = get_cache_dir() - resolver = SimpleSourceResolver(cache_dir=cache_dir) - result = await resolver.resolve(uri) + result = await GitSourceHandler().update( + parse_uri(f"git+{url}@{ref}"), + cache_dir, + ) logger.debug(f"Updated {module_id}@{ref} to {result.active_path}") diff --git a/tests/test_module_cache.py b/tests/test_module_cache.py new file mode 100644 index 00000000..e235ceaf --- /dev/null +++ b/tests/test_module_cache.py @@ -0,0 +1,309 @@ +"""Regression coverage for Foundation-keyed module cache refreshes.""" + +from __future__ import annotations + +import subprocess +from io import StringIO +from pathlib import Path + +import pytest +from click.testing import CliRunner +from rich.console import Console + +from amplifier_app_cli.utils import module_cache +from amplifier_app_cli.utils.module_cache import CachedModuleInfo +from amplifier_app_cli.utils.source_status import CachedGitStatus +from amplifier_app_cli.utils.update_executor import execute_selective_module_update + + +def _git(*args: str, cwd: Path) -> str: + """Run bounded fixture-only git commands without global configuration.""" + result = subprocess.run( + ["git", *args], + cwd=cwd, + check=True, + capture_output=True, + text=True, + timeout=10, + ) + return result.stdout.strip() + + +def _commit(repo: Path, message: str) -> str: + _git("add", "-A", cwd=repo) + _git("commit", "-q", "-m", message, cwd=repo) + return _git("rev-parse", "HEAD", cwd=repo) + + +def _make_remote( + tmp_path: Path, + repo_name: str, + *, + suffix: str, + bundle_name: str | None = None, + entry_point: str | None = None, + marker: str = "initial", +) -> tuple[Path, Path]: + """Create a local bare remote and a main worktree with independent identity.""" + worktree = tmp_path / f"work-{repo_name}-{suffix or 'no-suffix'}" + remote = tmp_path / f"{repo_name}{suffix}" + worktree.mkdir() + remote.mkdir() + + _git("init", "-q", "-b", "main", cwd=worktree) + _git("config", "user.name", "Module Cache Test", cwd=worktree) + _git("config", "user.email", "module-cache@example.invalid", cwd=worktree) + _git("init", "--bare", "-q", cwd=remote) + + if bundle_name: + (worktree / "bundle.md").write_text( + "---\n" + "bundle:\n" + f" name: {bundle_name}\n" + " version: 1.0.0\n" + "---\n", + encoding="utf-8", + ) + if entry_point: + (worktree / "pyproject.toml").write_text( + "[project]\n" + f'name = "{repo_name}"\n' + 'version = "0.0.1"\n' + '\n[project.entry-points."amplifier.modules"]\n' + f'"{entry_point}" = "example:mount"\n', + encoding="utf-8", + ) + + (worktree / "marker.txt").write_text(marker, encoding="utf-8") + _commit(worktree, "initial") + _git("remote", "add", "origin", remote.as_uri(), cwd=worktree) + _git("push", "-q", "-u", "origin", "main", cwd=worktree) + return worktree, remote + + +def _push_marker(worktree: Path, marker: str, *, branch: str = "main") -> None: + (worktree / "marker.txt").write_text(marker, encoding="utf-8") + _commit(worktree, marker) + _git("push", "-q", "origin", branch, cwd=worktree) + + +async def _seed_foundation_cache(uri: str, cache_dir: Path) -> Path: + """Populate an isolated cache through Foundation's real git resolver.""" + from amplifier_foundation.paths.resolution import parse_uri + from amplifier_foundation.sources.git import GitSourceHandler + + handler = GitSourceHandler() + parsed = parse_uri(uri) + resolved = await handler.resolve(parsed, cache_dir) + return resolved.source_root + + +@pytest.fixture +def cache_dir(tmp_path: Path) -> Path: + path = tmp_path / "cache" + path.mkdir() + return path + + +@pytest.mark.asyncio +@pytest.mark.parametrize("suffix", ["", ".git"]) +async def test_update_module_refreshes_bundle_by_source_identity( + tmp_path: Path, cache_dir: Path, suffix: str, monkeypatch: pytest.MonkeyPatch +): + """A declared bundle name need not match its repository name.""" + worktree, remote = _make_remote( + tmp_path, + "repository-name", + suffix=suffix, + bundle_name="declared-bundle-name", + ) + url = remote.as_uri() + cached_path = await _seed_foundation_cache(f"git+{url}@main", cache_dir) + assert (cached_path / "marker.txt").read_text(encoding="utf-8") == "initial" + + _push_marker(worktree, "updated") + monkeypatch.setattr(module_cache, "get_cache_dir", lambda: cache_dir) + + progress: list[tuple[str, str]] = [] + active_path = await module_cache.update_module( + url, + "main", + progress_callback=lambda label, phase: progress.append((label, phase)), + ) + + assert (active_path / "marker.txt").read_text(encoding="utf-8") == "updated" + assert progress == [ + ("repository-name", "clearing"), + ("repository-name", "downloading"), + ] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("repo_name", "entry_point"), + [ + ("repository-name", "provider-declared-id"), + ("amplifier-module-provider-example", "provider-example"), + ], +) +async def test_update_module_refreshes_module_when_entry_point_differs_from_repo( + tmp_path: Path, + cache_dir: Path, + repo_name: str, + entry_point: str, + monkeypatch: pytest.MonkeyPatch, +): + """Module entry-point IDs, including legacy repository paths, are not cache IDs.""" + worktree, remote = _make_remote( + tmp_path, + repo_name, + suffix=".git", + entry_point=entry_point, + ) + url = remote.as_uri() + await _seed_foundation_cache(f"git+{url}@main", cache_dir) + _push_marker(worktree, "updated") + monkeypatch.setattr(module_cache, "get_cache_dir", lambda: cache_dir) + + active_path = await module_cache.update_module(url, "main") + + assert (active_path / "marker.txt").read_text(encoding="utf-8") == "updated" + + +@pytest.mark.asyncio +async def test_update_module_keeps_other_source_and_ref_caches( + tmp_path: Path, cache_dir: Path, monkeypatch: pytest.MonkeyPatch +): + """Refreshing URL@main cannot remove same-name entries for other identities.""" + target_worktree, target_remote = _make_remote( + tmp_path, + "shared-bundle", + suffix=".git", + bundle_name="shared-bundle", + marker="target-old", + ) + _git("checkout", "-q", "-b", "other", cwd=target_worktree) + _push_marker(target_worktree, "other-old", branch="other") + _git("checkout", "-q", "main", cwd=target_worktree) + + _, other_remote = _make_remote( + tmp_path, + "different-repository", + suffix=".git", + bundle_name="shared-bundle", + marker="other-repository-old", + ) + target_url = target_remote.as_uri() + other_ref_path = await _seed_foundation_cache(f"git+{target_url}@other", cache_dir) + other_repo_path = await _seed_foundation_cache( + f"git+{other_remote.as_uri()}@main", cache_dir + ) + await _seed_foundation_cache(f"git+{target_url}@main", cache_dir) + + _push_marker(target_worktree, "target-new") + monkeypatch.setattr(module_cache, "get_cache_dir", lambda: cache_dir) + + active_path = await module_cache.update_module(target_url, "main") + + assert (active_path / "marker.txt").read_text(encoding="utf-8") == "target-new" + assert (other_ref_path / "marker.txt").read_text(encoding="utf-8") == "other-old" + assert (other_repo_path / "marker.txt").read_text( + encoding="utf-8" + ) == "other-repository-old" + + +class _FailingHandler: + async def update(self, parsed, cache_dir): + raise RuntimeError("refresh failed") + + +@pytest.mark.asyncio +async def test_update_module_propagates_foundation_refresh_failure( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + """The helper preserves the caller's existing failure-handling boundary.""" + monkeypatch.setattr("amplifier_foundation.sources.git.GitSourceHandler", _FailingHandler) + monkeypatch.setattr(module_cache, "get_cache_dir", lambda: tmp_path / "cache") + + with pytest.raises(RuntimeError, match="refresh failed"): + await module_cache.update_module("file:///missing", "main") + + +@pytest.mark.asyncio +async def test_selective_update_reports_handler_failure( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + """Selective execution records a real refresh failure rather than success.""" + monkeypatch.setattr("amplifier_foundation.sources.git.GitSourceHandler", _FailingHandler) + monkeypatch.setattr(module_cache, "get_cache_dir", lambda: tmp_path / "cache") + status = CachedGitStatus(name="provider-display", url="file:///missing", ref="main") + + result = await execute_selective_module_update([status]) + + assert not result.success + assert result.failed == ["provider-display"] + assert result.updated == [] + assert result.errors["provider-display"] == "refresh failed" + + +@pytest.mark.asyncio +async def test_selective_update_preserves_status_source_identity( + monkeypatch: pytest.MonkeyPatch, +): + """The selective executor passes the cached URL and ref to the refresh boundary.""" + seen: list[tuple[str, str]] = [] + + async def fake_update_module(url: str, ref: str, progress_callback=None) -> Path: + seen.append((url, ref)) + return Path("/isolated/cache") + + monkeypatch.setattr(module_cache, "update_module", fake_update_module) + status = CachedGitStatus( + name="provider-display", + url="file:///repository", + ref="release-branch", + ) + + result = await execute_selective_module_update([status]) + + assert result.success + assert result.updated == ["provider-display@release-branch"] + assert seen == [("file:///repository", "release-branch")] + + +def test_module_update_shows_failure_not_success_when_handler_raises( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + """The Click command reports a refresh error from the real update helper.""" + from importlib import import_module + + module_commands = import_module("amplifier_app_cli.commands.module") + + cache_dir = tmp_path / "cache" + cache_dir.mkdir() + output = StringIO() + cached = CachedModuleInfo( + module_id="provider-display", + module_type="provider", + ref="main", + sha="", + url="file:///missing", + is_mutable=True, + cached_at="", + cache_path=cache_dir, + ) + monkeypatch.setattr(module_cache, "get_cache_dir", lambda: cache_dir) + monkeypatch.setattr(module_cache, "find_cached_module", lambda module_id: cached) + monkeypatch.setattr("amplifier_foundation.sources.git.GitSourceHandler", _FailingHandler) + monkeypatch.setattr( + module_commands, + "console", + Console(file=output, force_terminal=False, color_system=None), + ) + + result = CliRunner().invoke(module_commands.module, ["update", cached.module_id]) + + rendered = output.getvalue() + assert result.exit_code == 0 + assert "Failed to update provider-display" in rendered + assert "Updated provider-display@main" not in rendered \ No newline at end of file