From f1746d21986de5e939b321acc87acfdd99f9ef96 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 14 Oct 2026 07:58:48 -0700 Subject: [PATCH 1/2] fix: update modules declared by app behaviors Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- AGENTS.md | 7 + amplifier_app_cli/commands/update.py | 338 +++++++++++- amplifier_app_cli/utils/include_graph.py | 205 +++++-- tests/test_bundle_commands.py | 43 +- tests/test_declared_module_updates.py | 657 +++++++++++++++++++++++ tests/test_transitive_include_updates.py | 10 +- tests/test_update_reporting.py | 12 +- 7 files changed, 1180 insertions(+), 92 deletions(-) create mode 100644 tests/test_declared_module_updates.py diff --git a/AGENTS.md b/AGENTS.md index 6247626a..c81d1967 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -105,6 +105,13 @@ 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`. +Ordinary updates must walk every cached root/app manifest before repository-row +deduplication, then check all five declared module kinds with runtime source +override precedence. Preserve exact module URIs (ref, `.git`, subdirectory); +refresh/install them through Foundation's selective module update, never load +them as bundles. Missing manifests are incomplete discovery, not current. +Keep `--check-only --force` read-only; run `tests/test_declared_module_updates.py`. + ## Persisted reminder display `ui.is_displayable_session_message()` is display-only: history and replay hide diff --git a/amplifier_app_cli/commands/update.py b/amplifier_app_cli/commands/update.py index e727aa9e..af2d411b 100644 --- a/amplifier_app_cli/commands/update.py +++ b/amplifier_app_cli/commands/update.py @@ -3,6 +3,7 @@ import asyncio from collections.abc import Iterable from dataclasses import dataclass +from dataclasses import replace import hashlib import nturl2path import os @@ -63,6 +64,21 @@ class TransitiveBundleStatus(_BundleStatus): """Ref is a commit SHA or version tag: reported, never refreshed.""" +@dataclass +class DeclaredModuleStatus(TransitiveBundleStatus): + """An exact module source, not a bundle URI to load or compose.""" + + module_id: str = "" + overridden_sources: tuple[str, ...] = () + + @property + def has_updates(self) -> bool: + return any( + source.has_update is True and not source.is_pinned and not source.error + for source in self.sources + ) + + SourceReportState = Literal[ "current", "download", @@ -219,6 +235,8 @@ def _check_failure_reason(error: object) -> str: message = str(error).lower() if message == "invalid source": return message + if message == "incomplete cached module discovery": + return "incomplete module discovery" if isinstance(error, TimeoutError) or "timeout" in message or "timed out" in message: return "timeout" if "rate limit" in message or "too many requests" in message or " 429" in message: @@ -465,23 +483,29 @@ def _bundle_action_lines( ) -> list[str]: """Group selected bundle targets by their existing, presentation-only plan.""" - counts: dict[SourceReportState, int] = {} - mixed: dict[str, int] = {} + counts: dict[tuple[SourceReportState, str], int] = {} + mixed: dict[tuple[str, str], int] = {} for name in bundle_names: + kind = ( + "module" + if isinstance(bundle_results[name], DeclaredModuleStatus) + else "bundle" + ) state = plan.get(name, "not_checked") if state == "mixed_actions": phrase = _bundle_action_phrase(bundle_results[name]) - mixed[phrase] = mixed.get(phrase, 0) + 1 + mixed[phrase, kind] = mixed.get((phrase, kind), 0) + 1 else: - counts[state] = counts.get(state, 0) + 1 + counts[state, kind] = counts.get((state, kind), 0) + 1 lines = [ - f"{_REPORT_STATE_LABELS[state]} {count} bundle{'s' if count != 1 else ''}" + f"{_REPORT_STATE_LABELS[state]} {count} {kind}{'s' if count != 1 else ''}" + for kind in ("bundle", "module") for state in ("download", "refresh_cache", "update") - if (count := counts.get(state, 0)) + if (count := counts.get((state, kind), 0)) ] lines.extend( - f"Process {count} bundle{'s' if count != 1 else ''} with {phrase}" - for phrase, count in mixed.items() + f"Process {count} {kind}{'s' if count != 1 else ''} with {phrase}" + for (phrase, kind), count in mixed.items() ) neutral_phrases = { "local_changes": "local changes", @@ -492,9 +516,10 @@ def _bundle_action_lines( "not_checked": "unconfirmed sources", } lines.extend( - f"Process {count} bundle{'s' if count != 1 else ''} with {phrase}" + f"Process {count} {kind}{'s' if count != 1 else ''} with {phrase}" + for kind in ("bundle", "module") for state, phrase in neutral_phrases.items() - if (count := counts.get(state, 0)) + if (count := counts.get((state, kind), 0)) ) return lines @@ -1077,6 +1102,14 @@ async def _check_all_bundle_status() -> dict[str, "BundleStatus"]: discovery = AppBundleDiscovery() registry = create_bundle_registry() + settings = AppSettings() + bundle_overrides = settings.get_bundle_sources() + if bundle_overrides: + from ..lib.bundle_loader.prepare import _build_include_source_resolver + + registry.set_include_source_resolver( + _build_include_source_resolver(bundle_overrides) + ) cache_dir = get_amplifier_home() / "cache" git_handler = GitSourceHandler() @@ -1150,6 +1183,19 @@ async def _check_bundle_uri(bundle_name: str, uri: str) -> BundleStatus: bundle_names = discovery.list_cached_root_bundles() results: dict[str, BundleStatus] = {} checked_uris: set[str] = set() + seeds: dict[str, str] = {} + root_discovery_failed = False + + # list_cached_root_bundles() deliberately collapses repository report rows. + # Walk every registered root manifest separately, before that filtering; + # non-roots remain reachable only through includes or explicit app seeds. + try: + for name in registry.list_registered(): + state = registry.get_state(name) + if state is not None and state.is_root is True and state.uri: + seeds[name] = state.uri + except Exception: # noqa: BLE001 - unreadable root metadata must not sink the report + root_discovery_failed = True for bundle_name in bundle_names: try: @@ -1160,6 +1206,7 @@ async def _check_bundle_uri(bundle_name: str, uri: str) -> BundleStatus: continue if not uri: continue + seeds[bundle_name] = uri checked_uris.add(uri) try: results[bundle_name] = await _check_bundle_uri(bundle_name, uri) @@ -1183,7 +1230,7 @@ async def _check_bundle_uri(bundle_name: str, uri: str) -> BundleStatus: # Scoped to the root URIs deliberately: two app entries that share a repo # are both things the user asked for by name, and stay independent. root_bases = {_strip_uri_fragment(uri) for uri in checked_uris} - for configured_source in AppSettings().get_app_bundles(): + for configured_source in settings.get_app_bundles(): if ( not isinstance(configured_source, str) or not configured_source.strip() @@ -1195,6 +1242,9 @@ async def _check_bundle_uri(bundle_name: str, uri: str) -> BundleStatus: ) continue uri = configured_source + # A behavior in a root's repository can declare different modules even + # when its repository status row is redundant. + seeds.setdefault(uri, uri) if uri in checked_uris or _strip_uri_fragment(uri) in root_bases: continue checked_uris.add(uri) @@ -1211,7 +1261,81 @@ async def _check_bundle_uri(bundle_name: str, uri: str) -> BundleStatus: # sources/git.py:697-709), the only thing that would ever move it is this # command. Skipping it here is what let a transitively-included @main # bundle sit indefinitely behind a green "All sources up to date". - results.update(await _check_transitive_bundle_status(checked_uris, results)) + results.update(await _check_transitive_bundle_status(set(seeds.values()), results)) + + from ..utils.include_graph import collect_declared_module_sources + + # Match runtime/config.py: general sources < module override < providers. + provider_sources = { + provider["module"]: provider["source"] + for provider in settings.get_provider_overrides() + if isinstance(provider, dict) and "module" in provider and "source" in provider + } + overrides = { + **settings.get_module_sources(), + **settings.get_source_overrides(), + **provider_sources, + } + labels = _bundle_display_names(seeds) + roots = {labels[key]: uri for key, uri in seeds.items()} + try: + declared, failures = await collect_declared_module_sources( + roots, registry=registry, cache_dir=cache_dir, overrides=overrides + ) + except Exception: # noqa: BLE001 - unreadable declarations must not sink the report + declared = {} + failures = {label: "Incomplete cached module discovery" for label in roots} + if root_discovery_failed: + failures.update({label: "Incomplete cached module discovery" for label in roots}) + + for uri, declaration in declared.items(): + try: + parsed = parse_uri(uri) + if git_handler.can_handle(parsed): + source_status = await git_handler.get_status(parsed, cache_dir) + else: + source_status = SourceStatus( + source_uri=uri, is_cached=True, has_update=None + ) + except Exception as exc: # noqa: BLE001 - one bad source must not sink the report + source_status = SourceStatus( + source_uri=uri, + is_cached=False, + has_update=None, + error=_check_failure_reason(exc), + ) + # Exact source identity, including ref, .git and subdirectory. A module + # row wins over a cache-only include row for the same exact source. + results[uri] = DeclaredModuleStatus( + bundle_name=declaration.module_id, + bundle_source=uri, + sources=[source_status], + module_id=declaration.module_id, + overridden_sources=declaration.overridden_sources, + included_by=declaration.declared_by, + included_under=declaration.root, + is_pinned=source_status.is_pinned, + ) + + for root, error in failures.items(): + root_uri = roots[root] + owner = next( + ( + status + for status in results.values() + if not isinstance(status, DeclaredModuleStatus) + and status.bundle_source + and _strip_uri_fragment(status.bundle_source) + == _strip_uri_fragment(root_uri) + ), + None, + ) + if owner is not None: + owner.sources.append( + SourceStatus( + source_uri="", is_cached=False, has_update=None, error=error + ) + ) return results @@ -1236,16 +1360,27 @@ async def _check_transitive_bundle_status( cache_dir = get_amplifier_home() / "cache" registry = create_bundle_registry() + bundle_overrides = AppSettings().get_bundle_sources() + if bundle_overrides: + from ..lib.bundle_loader.prepare import _build_include_source_resolver + + registry.set_include_source_resolver( + _build_include_source_resolver(bundle_overrides) + ) # Seed the walk with the label each direct row already displays, so a # transitive row can name a parent the user can actually find in the table. - seed_labels = _bundle_display_names(direct_results.keys()) - roots = { - seed_labels.get(key, key): status.bundle_source + seeds = { + key: status.bundle_source for key, status in direct_results.items() if status.bundle_source } + # Include seeds whose direct report row was deduplicated against a root. + seeds.update({uri: uri for uri in checked_uris if uri not in seeds.values()}) + seed_labels = _bundle_display_names(seeds) + roots = {seed_labels[key]: uri for key, uri in seeds.items()} + known = {_strip_uri_fragment(uri) for uri in checked_uris} known.update( _strip_uri_fragment(s.bundle_source) @@ -1567,7 +1702,15 @@ def _show_concise_report( if bundle_results: console.print() active_bundle = _get_active_bundle_name() - table = Table(title="Bundles", show_header=True, header_style="bold cyan") + title = ( + "Bundles and Modules" + if any( + isinstance(status, DeclaredModuleStatus) + for status in bundle_results.values() + ) + else "Bundles" + ) + table = Table(title=title, show_header=True, header_style="bold cyan") table.add_column("Name", style="green") table.add_column("Cached", style="dim", justify="right") table.add_column("Remote", style="dim", justify="right") @@ -1598,6 +1741,8 @@ def _show_concise_report( # Add "(active)" marker if this is the active bundle display_name = display_names[bundle_name] + if isinstance(bundle_status, DeclaredModuleStatus): + display_name = f"{bundle_status.module_id} ({display_name})" if active_bundle and bundle_name == active_bundle: display_name = f"{display_name} [green](active)[/green]" @@ -1817,8 +1962,14 @@ def _show_verbose_report( included_by = getattr(status, "included_by", "") if included_by: title_suffix += f" (via {included_by})" + heading = ( + "Module" if isinstance(status, DeclaredModuleStatus) else "Bundle" + ) + display_name = display_names[bundle_name] + if isinstance(status, DeclaredModuleStatus): + display_name = f"{status.module_id} ({display_name})" console.print( - f"[bold cyan]Bundle: {display_names[bundle_name]}" + f"[bold cyan]{heading}: {display_name}" f"{title_suffix}[/bold cyan]" ) console.print( @@ -1837,6 +1988,8 @@ def _show_verbose_report( elif "@" in source_name: source_name = source_name.split("@")[0] + if isinstance(status, DeclaredModuleStatus): + source_name = status.module_id source_state = _classify_source_status( source, has_local_changes=bool( @@ -1851,11 +2004,13 @@ def _show_verbose_report( local_path=( source.source_uri if source.source_uri.startswith("file://") + and not isinstance(status, DeclaredModuleStatus) else None ), remote_url=( None if source.source_uri.startswith("file://") + or isinstance(status, DeclaredModuleStatus) else source.source_uri ), ) @@ -2000,6 +2155,79 @@ def _refresh_skills_cache(console: Console) -> None: console.print(f" Skills: {refreshed} cache(s) refreshed") +def _deduplicate_declared_module_caches( + report, bundle_results: dict[str, _BundleStatus] +) -> None: + """Let exact declared module actions own their legacy repository refresh. + + Cache identity comes from Foundation's URL + ref key, never the displayed + module name. Legacy root-only entries cannot describe subdirectory installs. + """ + from amplifier_foundation.paths.resolution import parse_uri + from amplifier_foundation.sources.git import GitSourceHandler + + handler = GitSourceHandler() + cache_dir = get_amplifier_home() / "cache" + selected: dict[Path, set[str]] = {} + overridden: dict[Path, set[str]] = {} + effective_caches: set[Path] = set() + + def cache_key(uri: str) -> Path | None: + try: + parsed = parse_uri(uri) + if handler.can_handle(parsed): + return handler._get_cache_path(parsed, cache_dir) + except (ValueError, TypeError, OSError): + # Failed checks remain visible; they are not legacy targets. + return None + return None + + for status in bundle_results.values(): + if not isinstance(status, DeclaredModuleStatus): + continue + for original in status.overridden_sources: + key = cache_key(original) + if key is not None: + overridden.setdefault(key, set()).add(original) + uri = status.bundle_source + if not uri: + continue + key = cache_key(uri) + if key is not None: + effective_caches.add(key) + if status.has_updates: + selected.setdefault(key, set()).add(uri) + + # A replaced source is not an active module target. Do not let an old + # root-only cache scan refresh it behind a local (or different remote) + # override, unless another declaration still uses that cache. + for key, uris in overridden.items(): + if key not in effective_caches: + selected.setdefault(key, set()).update(uris) + + remaining = [] + for status in report.cached_git_sources: + if not status.url: + remaining.append(status) + continue + try: + uri = status.url + if not uri.startswith(("git+", "git://")): + uri = f"git+{uri}" + parsed = parse_uri(uri) + if status.ref: + parsed = replace(parsed, ref=status.ref) + matches = selected.get(handler._get_cache_path(parsed, cache_dir), set()) + if matches and (not parsed.subpath or uri in matches): + continue + except (ValueError, TypeError, OSError): + # An unrecognized legacy source is not ours to discard. + remaining.append(status) + continue + remaining.append(status) + report.cached_git_sources = remaining + + @click.command() @click.option("--check-only", is_flag=True, help="Check for updates without installing") @click.option("--yes", "-y", is_flag=True, help="Skip confirmations") @@ -2013,7 +2241,7 @@ def update(check_only: bool, yes: bool, force: bool, verbose: bool): Checks all sources (local files and cached git) and executes updates. """ # Check for updates with status messages - if force: + if force and not check_only: console.print("Force update mode...") # Clear regenerable cache so everything is fetched fresh from ..utils.cache_management import clear_all_regenerable @@ -2071,8 +2299,12 @@ async def _check_sources(): if not force: console.print(" Checking bundles...") bundle_results = asyncio.run(_check_all_bundle_status()) + _deduplicate_declared_module_caches(report, bundle_results) bundle_plan = _bundle_report_plan(bundle_results) bundle_labels = _bundle_display_names(bundle_results.keys()) + for name, status in bundle_results.items(): + if isinstance(status, DeclaredModuleStatus): + bundle_labels[name] = f"{status.module_id} ({bundle_labels[name]})" has_bundle_updates = ( any(s.has_updates for s in bundle_results.values()) if bundle_results else False ) @@ -2221,6 +2453,17 @@ def _on_update_progress(name: str, phase: str) -> None: bundle_updated: list[str] = [] bundle_failed: list[str] = [] bundle_errors: dict[str, str] = {} + for name, status in bundle_results.items(): + if not isinstance(status, DeclaredModuleStatus): + continue + errors = { + _check_failure_reason(source.error) + for source in status.sources + if source.error + } + if errors: + bundle_failed.append(name) + bundle_errors[name] = ", ".join(sorted(errors)) if has_bundle_updates: from amplifier_foundation import load_bundle @@ -2241,10 +2484,38 @@ def _on_update_progress(name: str, phase: str) -> None: for bundle_name in bundles_to_update: try: - _on_update_progress(update_labels[bundle_name], "updating_bundle") bundle_status_obj = bundle_results[bundle_name] + _on_update_progress( + update_labels[bundle_name], + "updating" + if isinstance(bundle_status_obj, DeclaredModuleStatus) + else "updating_bundle", + ) source_uri = bundle_status_obj.bundle_source + if isinstance(bundle_status_obj, DeclaredModuleStatus): + from amplifier_foundation import Bundle + + source_only = Bundle( + name=bundle_status_obj.module_id, + tools=[ + { + "module": bundle_status_obj.module_id, + "source": source_uri, + } + ], + ) + asyncio.run( + update_bundle( + source_only, + cache_dir=transitive_cache_dir, + selective=[source_uri], + install_deps=True, + ) + ) + bundle_updated.append(bundle_name) + continue + # A transitive source is a cache, not a bundle we compose # here: load_bundle() would resolve and re-register a # bundle nobody asked for. Refresh the cache with the same @@ -2270,10 +2541,14 @@ def _on_update_progress(name: str, phase: str) -> None: # Refresh bundle sources _on_update_progress(update_labels[bundle_name], "refreshing_bundle") - asyncio.run(update_bundle(bundle_obj)) + asyncio.run(update_bundle(bundle_obj, selective=[source_uri])) bundle_updated.append(bundle_name) except Exception as exc: - bundle_errors[bundle_name] = str(exc) + bundle_errors[bundle_name] = ( + _check_failure_reason(exc) + if isinstance(bundle_results[bundle_name], DeclaredModuleStatus) + else str(exc) + ) bundle_failed.append(bundle_name) # Refresh any stale skills caches @@ -2296,9 +2571,14 @@ def _on_update_progress(name: str, phase: str) -> None: for item in result.staged: console.print(f" [yellow]→[/yellow] {item} [dim](staged)[/dim]") for bundle_name in bundle_updated: + kind = ( + "module" + if isinstance(bundle_results[bundle_name], DeclaredModuleStatus) + else "bundle" + ) console.print( f" [green]✓[/green] " - f"{_bundle_completion_verb(bundle_plan.get(bundle_name))} bundle: " + f"{_bundle_completion_verb(bundle_plan.get(bundle_name))} {kind}: " f"{escape_markup(bundle_labels[bundle_name])}" ) for msg in result.messages: @@ -2313,9 +2593,14 @@ def _on_update_progress(name: str, phase: str) -> None: for item in result.staged: console.print(f" [yellow]→[/yellow] {item} [dim](staged)[/dim]") for bundle_name in bundle_updated: + kind = ( + "module" + if isinstance(bundle_results[bundle_name], DeclaredModuleStatus) + else "bundle" + ) console.print( f" [green]✓[/green] " - f"{_bundle_completion_verb(bundle_plan.get(bundle_name))} bundle: " + f"{_bundle_completion_verb(bundle_plan.get(bundle_name))} {kind}: " f"{escape_markup(bundle_labels[bundle_name])}" ) for item in result.failed: @@ -2323,9 +2608,14 @@ def _on_update_progress(name: str, phase: str) -> None: console.print(f" [red]✗[/red] {item}: {escape_markup(error)}") for bundle_name in bundle_failed: error = bundle_errors.get(bundle_name, "Unknown error") + kind = ( + "module" + if isinstance(bundle_results[bundle_name], DeclaredModuleStatus) + else "bundle" + ) console.print( f" [red]✗[/red] " - f"{_bundle_failure_verb(bundle_plan.get(bundle_name))} bundle: " + f"{_bundle_failure_verb(bundle_plan.get(bundle_name))} {kind}: " f"{escape_markup(bundle_labels[bundle_name])}: {escape_markup(error)}" ) for msg in result.messages: diff --git a/amplifier_app_cli/utils/include_graph.py b/amplifier_app_cli/utils/include_graph.py index 33f21c2d..8e75e74c 100644 --- a/amplifier_app_cli/utils/include_graph.py +++ b/amplifier_app_cli/utils/include_graph.py @@ -39,13 +39,15 @@ from __future__ import annotations import logging +from collections.abc import AsyncIterator from dataclasses import dataclass +from dataclasses import replace from pathlib import Path from typing import TYPE_CHECKING from typing import Mapping if TYPE_CHECKING: - from amplifier_foundation import BundleRegistry + from amplifier_foundation import Bundle, BundleRegistry from amplifier_foundation.sources.protocol import SourceStatus logger = logging.getLogger(__name__) @@ -144,6 +146,8 @@ def _local_path_for(uri: str, cache_dir: Path) -> Path | None: if parsed.is_file: path = Path(parsed.path) + if parsed.subpath: + path = path / parsed.subpath return path if path.exists() else None return None @@ -153,33 +157,26 @@ def _is_git_uri(uri: str) -> bool: return uri.startswith("git+") or uri.startswith("git://") -async def collect_transitive_git_sources( +async def _iter_cached_bundles( roots: Mapping[str, str], *, - registry: "BundleRegistry", + registry: BundleRegistry, cache_dir: Path, - known_uris: set[str] | None = None, max_depth: int = 16, -) -> dict[str, TransitiveSource]: - """Walk ``includes:`` from *roots* and return the git sources found. - - Args: - roots: Display name -> configured URI for every direct update target. - registry: Loader registry -- supplies include parsing/resolution and - the bundle-file parser. Namespace includes resolve against its - state, so pass the same registry the CLI builds elsewhere. - cache_dir: Cache root (``/cache``). - known_uris: Fragment-stripped URIs already covered by a direct row. - Anything here is skipped -- it is not "transitive", it is the - source that was already going to be checked. - max_depth: Belt-and-braces bound on graph depth. Cycles are already - handled by the visited set; this only caps a pathological chain. - - Returns: - Fragment-stripped URI -> TransitiveSource, in discovery order. + failures: dict[str, str] | None = None, +) -> AsyncIterator[tuple[str, str, str, Bundle | None]]: + """Yield root, immediate parent, full URI and cached manifest (if readable). + + Both collectors use this bounded, read-only walk. A missing manifest still + yields its URI so include status checking retains its existing behavior. + Failure text deliberately contains neither configured URIs nor exceptions. """ - known = set(known_uris or set()) - found: dict[str, TransitiveSource] = {} + from amplifier_foundation.paths.resolution import parse_uri + + def failed(root: str) -> None: + if failures is not None: + failures[root] = "Incomplete cached module discovery" + logger.debug("Could not completely read cached bundle declarations") # Visited on the FULL uri (fragment included): two behaviors in one repo # are different files with different includes, so collapsing them on the @@ -197,59 +194,145 @@ async def collect_transitive_git_sources( while queue: root_label, label, uri, depth = queue.pop(0) if depth >= max_depth: - logger.debug(f"Include walk hit max depth at: {uri}") - continue - - path = _local_path_for(uri, cache_dir) - if path is None: + failed(root_label) + yield root_label, label, uri, None continue try: + path = _local_path_for(uri, cache_dir) + if path is None: + failed(root_label) + yield root_label, label, uri, None + continue bundle = await registry._load_from_path(path) - except Exception as exc: # noqa: BLE001 - an unreadable bundle is not fatal - logger.debug(f"Could not read includes from {uri}: {exc}") + except Exception: # noqa: BLE001 - an unreadable bundle is not fatal + failed(root_label) + yield root_label, label, uri, None continue + yield root_label, label, uri, bundle parent_label = bundle.name or label for raw_include in bundle.includes or []: - spec = registry._parse_include(raw_include) - if not spec: - continue - try: + spec = registry._parse_include(raw_include) + if not spec: + failed(root_label) + continue resolved = registry._resolve_include_source(spec) - except Exception as exc: # noqa: BLE001 - logger.debug(f"Could not resolve include '{spec}': {exc}") - continue - - if not resolved: - continue - - # A plain name is a registry alias; the loader lets _load_single - # look it up, so look it up the same way here. - if "://" not in resolved and not resolved.startswith("git+"): - looked_up = registry.find(resolved) - if not looked_up: + if not resolved: + failed(root_label) continue - resolved = looked_up + # A plain name is a registry alias, not a filesystem source. + if ( + "://" not in resolved + and not resolved.startswith("git+") + and not parse_uri(resolved).is_file + ): + resolved = registry.find(resolved) + if not resolved: + failed(root_label) + continue + except Exception: # noqa: BLE001 + failed(root_label) + continue if resolved in visited: continue visited.add(resolved) + queue.append((root_label, parent_label, resolved, depth + 1)) - if _is_git_uri(resolved): - key = strip_uri_fragment(resolved) - if key not in known and key not in found: - found[key] = TransitiveSource( - uri=key, parent=parent_label, root=root_label, via=resolved - ) - queue.append((root_label, parent_label, resolved, depth + 1)) +async def collect_transitive_git_sources( + roots: Mapping[str, str], + *, + registry: "BundleRegistry", + cache_dir: Path, + known_uris: set[str] | None = None, + max_depth: int = 16, +) -> dict[str, TransitiveSource]: + """Return fragment-stripped included git sources, excluding direct targets. + Visit full URIs (including fragments) and reuse registry include resolution. + Missing caches are never downloaded; their include URIs are still returned. + """ + known = set(known_uris or set()) + root_uris = set(roots.values()) + found: dict[str, TransitiveSource] = {} + async for root, parent, uri, _ in _iter_cached_bundles( + roots, registry=registry, cache_dir=cache_dir, max_depth=max_depth + ): + if uri in root_uris or not _is_git_uri(uri): + continue + key = strip_uri_fragment(uri) + if key not in known and key not in found: + found[key] = TransitiveSource(uri=key, parent=parent, root=root, via=uri) return found +@dataclass(frozen=True) +class DeclaredModuleSource: + """First declaration plus replaced URIs, so legacy caches honor overrides.""" + + module_id: str + declared_by: str + root: str + overridden_sources: tuple[str, ...] = () + + +async def collect_declared_module_sources( + roots: Mapping[str, str], + *, + registry: BundleRegistry, + cache_dir: Path, + overrides: Mapping[str, str] | None = None, +) -> tuple[dict[str, DeclaredModuleSource], dict[str, str]]: + """Return exact effective URI -> declaration metadata, and root failures. + + Inspect the same five module locations as Foundation's source collector. + Module sources are declarations, never bundle manifests to walk. First + provenance wins when several module IDs share the same effective source. + """ + found: dict[str, DeclaredModuleSource] = {} + failures: dict[str, str] = {} + overrides = overrides or {} + async for root, parent, _, bundle in _iter_cached_bundles( + roots, registry=registry, cache_dir=cache_dir, failures=failures + ): + if bundle is None: + continue + session = bundle.session or {} + declarations = [ + session.get("orchestrator"), + session.get("context"), + *(bundle.providers or []), + *(bundle.tools or []), + *(bundle.hooks or []), + ] + for declaration in declarations: + if not isinstance(declaration, dict): + continue + module_id = declaration.get("module") + if not isinstance(module_id, str): + continue + uri = overrides.get(module_id, declaration.get("source")) + if not isinstance(uri, str) or not uri: + continue + entry = found.setdefault( + uri, DeclaredModuleSource(module_id, bundle.name or parent, root) + ) + original = declaration.get("source") + if ( + isinstance(original, str) + and original != uri + and original not in entry.overridden_sources + ): + found[uri] = replace( + entry, overridden_sources=(*entry.overridden_sources, original) + ) + return found, failures + + async def check_transitive_sources( sources: Mapping[str, TransitiveSource], *, @@ -264,6 +347,7 @@ async def check_transitive_sources( """ from amplifier_foundation.paths.resolution import parse_uri from amplifier_foundation.sources.git import GitSourceHandler + from amplifier_foundation.sources.protocol import SourceStatus git_handler = GitSourceHandler() results: dict[str, TransitiveStatus] = {} @@ -274,9 +358,14 @@ async def check_transitive_sources( if not git_handler.can_handle(parsed): continue status = await git_handler.get_status(parsed, cache_dir) - except Exception as exc: # noqa: BLE001 - one bad source must not sink the report - logger.debug(f"Status check failed for {source.uri}: {exc}") - continue + except Exception: # noqa: BLE001 - one bad source must not sink the report + logger.debug("Transitive source status check failed") + status = SourceStatus( + source_uri=source.uri, + is_cached=False, + has_update=None, + error="status check failed", + ) results[key] = TransitiveStatus(source=source, status=status) return results @@ -312,9 +401,11 @@ async def refresh_transitive_source(uri: str, *, cache_dir: Path) -> None: __all__ = [ + "DeclaredModuleSource", "TransitiveSource", "TransitiveStatus", "check_transitive_sources", + "collect_declared_module_sources", "collect_transitive_git_sources", "refresh_transitive_source", "strip_uri_fragment", diff --git a/tests/test_bundle_commands.py b/tests/test_bundle_commands.py index 3348b17b..d10619f2 100644 --- a/tests/test_bundle_commands.py +++ b/tests/test_bundle_commands.py @@ -218,7 +218,13 @@ async def test_global_update_checks_app_only_sources_by_exact_uri(monkeypatch): monkeypatch.setattr( update_module, "AppSettings", - lambda: SimpleNamespace(get_app_bundles=lambda: app_uris), + lambda: SimpleNamespace( + get_app_bundles=lambda: app_uris, + get_bundle_sources=dict, + get_module_sources=dict, + get_source_overrides=dict, + get_provider_overrides=list, + ), ) async def fake_get_status(self, parsed, cache_dir): @@ -259,7 +265,13 @@ def find_bundle(bundle_name): monkeypatch.setattr( update_module, "AppSettings", - lambda: SimpleNamespace(get_app_bundles=lambda: []), + lambda: SimpleNamespace( + get_app_bundles=list, + get_bundle_sources=dict, + get_module_sources=dict, + get_source_overrides=dict, + get_provider_overrides=list, + ), ) async def fake_get_status(self, parsed, cache_dir): @@ -296,7 +308,13 @@ async def test_global_update_reports_direct_app_check_failure_by_exact_uri(monke monkeypatch.setattr( update_module, "AppSettings", - lambda: SimpleNamespace(get_app_bundles=lambda: [failing_uri]), + lambda: SimpleNamespace( + get_app_bundles=lambda: [failing_uri], + get_bundle_sources=dict, + get_module_sources=dict, + get_source_overrides=dict, + get_provider_overrides=list, + ), ) async def failing_get_status(self, parsed, cache_dir): @@ -349,7 +367,8 @@ async def fake_load_bundle(uri, *, auto_include): loaded_uris.append(uri) return SimpleNamespace() - async def fake_update_bundle(bundle): + async def fake_update_bundle(bundle, *, selective): + assert selective == [_FRAGMENT_URI] updated_bundles.append(bundle) monkeypatch.setattr(update_module, "check_all_sources", fake_check_all_sources) @@ -409,7 +428,13 @@ async def test_global_update_collapses_an_app_subdirectory_into_its_root_bundle( monkeypatch.setattr( update_module, "AppSettings", - lambda: SimpleNamespace(get_app_bundles=lambda: [_FRAGMENT_URI]), + lambda: SimpleNamespace( + get_app_bundles=lambda: [_FRAGMENT_URI], + get_bundle_sources=dict, + get_module_sources=dict, + get_source_overrides=dict, + get_provider_overrides=list, + ), ) async def fake_get_status(self, parsed, cache_dir): @@ -445,7 +470,13 @@ async def test_global_update_keeps_an_app_subdirectory_on_a_different_ref(monkey monkeypatch.setattr( update_module, "AppSettings", - lambda: SimpleNamespace(get_app_bundles=lambda: [other_ref_uri]), + lambda: SimpleNamespace( + get_app_bundles=lambda: [other_ref_uri], + get_bundle_sources=dict, + get_module_sources=dict, + get_source_overrides=dict, + get_provider_overrides=list, + ), ) async def fake_get_status(self, parsed, cache_dir): diff --git a/tests/test_declared_module_updates.py b/tests/test_declared_module_updates.py new file mode 100644 index 00000000..3cad2ac3 --- /dev/null +++ b/tests/test_declared_module_updates.py @@ -0,0 +1,657 @@ +"""Ordinary update discovers cached declarations and installs exact module sources.""" + +from __future__ import annotations + +import asyncio +import io +import json +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock + +import amplifier_foundation +import pytest +import yaml +from amplifier_foundation import Bundle, BundleRegistry +from amplifier_foundation.modules.activator import ModuleActivator +from amplifier_foundation.paths.resolution import parse_uri +from amplifier_foundation.sources.git import GitSourceHandler +from amplifier_foundation.sources.protocol import SourceStatus +from click.testing import CliRunner +from rich.console import Console + +from amplifier_app_cli.commands import update as updater +from amplifier_app_cli.lib.bundle_loader import discovery as discovery_module +from amplifier_app_cli.lib.settings import AppSettings +from amplifier_app_cli.paths import create_bundle_registry +from amplifier_app_cli.utils.include_graph import collect_declared_module_sources +from amplifier_app_cli.utils.source_status import CachedGitStatus, UpdateReport +from amplifier_app_cli.utils.update_executor import ExecutionResult + +ROOT = "git+https://example.invalid/team/host.git@main" +APP = ROOT + "#subdirectory=behaviors/app.yaml" +CHILD = "git+https://example.invalid/team/included.git@main" +MODULES = "git+https://example.invalid/team/modules.git@feature/update" +TOOL = MODULES + "#subdirectory=tools/worker" +HOOK = MODULES + "#subdirectory=hooks/watch" + + +def _uri(parsed) -> str: + result = f"{parsed.scheme}://{parsed.host}{parsed.path}" + if parsed.ref: + result += f"@{parsed.ref}" + if parsed.subpath: + result += f"#subdirectory={parsed.subpath}" + return result + + +@pytest.fixture +def scenario(tmp_path, monkeypatch): + """Keep real manifest parsing and Foundation updating, fake all external edges.""" + home = tmp_path / "home" + home.mkdir() + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.setenv("USERPROFILE", str(home)) + monkeypatch.setenv("AMPLIFIER_HOME", str(home)) + monkeypatch.delenv("AMPLIFIER_SOURCE_STORE", raising=False) + cache = home / "cache" + registry = BundleRegistry(home=home) + registry.register({"root": ROOT, "child": CHILD}) + registry.get_state("child").is_root = False + handler = GitSourceHandler() + real_get_status = GitSourceHandler.get_status + + def active_path(uri): + parsed = parse_uri(uri) + base = handler._get_cache_path(parsed, cache) + return base / parsed.subpath if parsed.subpath else base + + def write_bundle(uri, config): + path = active_path(uri) + if not parse_uri(uri).subpath: + path = path / "bundle.md" + path.parent.mkdir(parents=True, exist_ok=True) + data = {"bundle": {"name": "shared-metadata", "version": "1.0.0"}, **config} + text = yaml.safe_dump(data, sort_keys=False) + if path.suffix == ".md": + text = f"---\n{text}---\n" + path.write_text(text, encoding="utf-8") + return path + + write_bundle(ROOT, {"includes": [{"bundle": "child"}]}) + write_bundle( + APP, + { + "includes": [{"bundle": CHILD}, {"bundle": CHILD}], + "tools": [{"module": "tool-worker", "source": TOOL}], + "hooks": [{"module": "hooks-watch", "source": HOOK}], + }, + ) + write_bundle(CHILD, {"includes": [{"bundle": ROOT}]}) + for uri in (TOOL, HOOK): + path = active_path(uri) + path.mkdir(parents=True, exist_ok=True) + (path / "pyproject.toml").write_text( + '[project]\nname = "fixture-module"\nversion = "1.0.0"\n', + encoding="utf-8", + ) + + settings = MagicMock(spec=AppSettings) + settings.get_app_bundles.return_value = [APP] + settings.get_bundle_sources.return_value = {} + settings.get_module_sources.return_value = {} + settings.get_source_overrides.return_value = {} + settings.get_provider_overrides.return_value = [] + settings.get_added_bundles.return_value = {} + monkeypatch.setattr(updater, "AppSettings", lambda: settings) + monkeypatch.setattr(updater, "create_bundle_registry", lambda: registry) + monkeypatch.setattr( + updater, + "AppBundleDiscovery", + lambda: SimpleNamespace(list_cached_root_bundles=lambda: ["root"]), + ) + monkeypatch.setattr( + updater, + "create_config_manager", + lambda: SimpleNamespace(get_merged_settings=dict), + ) + behind = {TOOL, HOOK} + errors = {} + checked = [] + + async def status(self, parsed, cache_dir): + assert cache_dir == cache + uri = _uri(parsed) + checked.append(uri) + if uri in errors: + raise errors[uri] + return SourceStatus( + source_uri=uri, + is_cached=active_path(uri).exists(), + cached_ref=parsed.ref, + cached_commit="a" * 40, + remote_commit=("b" if uri in behind else "a") * 40, + has_update=uri in behind, + ) + + refreshed = [] + + async def refresh(self, parsed, cache_dir): + assert cache_dir == cache + uri = _uri(parsed) + refreshed.append(uri) + return SimpleNamespace(active_path=active_path(uri)) + + monkeypatch.setattr(GitSourceHandler, "get_status", status) + monkeypatch.setattr(GitSourceHandler, "update", refresh) + forbidden = AsyncMock(side_effect=AssertionError("unexpected resolve/load/prepare")) + monkeypatch.setattr(GitSourceHandler, "resolve", forbidden) + monkeypatch.setattr(BundleRegistry, "load", forbidden) + monkeypatch.setattr(Bundle, "prepare", forbidden) + monkeypatch.setattr(amplifier_foundation, "load_bundle", forbidden) + installs = AsyncMock() + monkeypatch.setattr(ModuleActivator, "_install_dependencies", installs) + apply = AsyncMock(wraps=amplifier_foundation.update_bundle) + monkeypatch.setattr(amplifier_foundation, "update_bundle", apply) + report = UpdateReport(local_file_sources=[], cached_git_sources=[]) + monkeypatch.setattr(updater, "check_all_sources", AsyncMock(return_value=report)) + execute = AsyncMock( + return_value=ExecutionResult(success=True, updated=[], messages=[]) + ) + monkeypatch.setattr(updater, "execute_updates", execute) + monkeypatch.setattr( + "amplifier_app_cli.utils.umbrella_discovery.discover_umbrella_source", + lambda: None, + ) + skills = MagicMock() + save = MagicMock() + clear = MagicMock(side_effect=AssertionError("unexpected cache clear")) + monkeypatch.setattr(updater, "_refresh_skills_cache", skills) + monkeypatch.setattr(updater, "save_update_last_check", save) + monkeypatch.setattr( + "amplifier_app_cli.utils.cache_management.clear_all_regenerable", clear + ) + output = io.StringIO() + monkeypatch.setattr(updater, "console", Console(file=output, width=240)) + + def invoke(*args, input=None): + output.seek(0) + output.truncate() + result = CliRunner().invoke(updater.update, list(args), input=input) + assert result.exit_code == 0, (result.exception, output.getvalue()) + return output.getvalue() + + return SimpleNamespace( + cache=cache, + registry=registry, + settings=settings, + behind=behind, + errors=errors, + checked=checked, + refreshed=refreshed, + installs=installs, + apply=apply, + report=report, + execute=execute, + forbidden=forbidden, + skills=skills, + save=save, + clear=clear, + active_path=active_path, + write_bundle=write_bundle, + invoke=invoke, + real_get_status=real_get_status, + ) + + +def test_current_root_same_repo_app_updates_and_installs_both_module_subdirectories( + scenario, +): + output = scenario.invoke("--check-only") + assert "Update 2 modules" in output + assert "Update 2 bundles" not in output + assert "All sources up to date" not in output + assert not scenario.refreshed + declined = scenario.invoke(input="n\n") + assert "Update cancelled" in declined + assert not scenario.refreshed + + output = scenario.invoke("--yes") + assert "Update complete" in output + assert "Updated module: tool-worker" in output + assert "Updated module: hooks-watch" in output + assert scenario.refreshed == [TOOL, HOOK] + assert [call.args[0] for call in scenario.installs.await_args_list] == [ + scenario.active_path(TOOL), + scenario.active_path(HOOK), + ] + assert scenario.apply.await_count == 2 + for call, uri, module_id in zip( + scenario.apply.await_args_list, + (TOOL, HOOK), + ("tool-worker", "hooks-watch"), + strict=True, + ): + source_only = call.args[0] + assert not getattr(source_only, "_source_uri", None) + assert source_only.tools == [{"module": module_id, "source": uri}] + assert call.kwargs == { + "cache_dir": scenario.cache, + "selective": [uri], + "install_deps": True, + } + scenario.forbidden.assert_not_called() + + +def test_all_five_module_kinds_and_shared_exact_source_have_first_provenance(scenario): + orchestrator = MODULES + "#subdirectory=orchestrator" + context = MODULES + "#subdirectory=context" + provider = MODULES + "#subdirectory=provider" + scenario.write_bundle( + ROOT, + { + "includes": [{"bundle": "child"}], + "session": { + "orchestrator": {"module": "loop-worker", "source": orchestrator}, + "context": {"module": "context-worker", "source": context}, + }, + "providers": [{"module": "provider-worker", "source": provider}], + }, + ) + scenario.write_bundle( + CHILD, + { + "includes": [{"bundle": ROOT}], + "tools": [{"module": "tool-second-id", "source": TOOL}], + }, + ) + found, failures = asyncio.run( + collect_declared_module_sources( + {"root": ROOT, "app": APP}, + registry=scenario.registry, + cache_dir=scenario.cache, + ) + ) + assert not failures + assert set(found) == {orchestrator, context, provider, TOOL, HOOK} + assert found[TOOL].module_id == "tool-worker" + assert found[TOOL].declared_by == "shared-metadata" + assert found[TOOL].root == "app" + statuses = asyncio.run(updater._check_all_bundle_status()) + assert APP not in statuses # Repo row dedup must not suppress this manifest. + assert { + uri + for uri, status in statuses.items() + if isinstance(status, updater.DeclaredModuleStatus) + } == set(found) + scenario.invoke("--yes") + assert scenario.refreshed == [TOOL, HOOK] + assert scenario.apply.await_count == 2 + + +def test_module_override_precedence_and_local_source_never_refresh_original( + scenario, tmp_path +): + local = tmp_path / "local-module" + local.mkdir() + specific = MODULES + "#subdirectory=specific" + provider = MODULES + "#subdirectory=provider-choice" + scenario.settings.get_module_sources.return_value = { + "tool-worker": specific, + "hooks-watch": specific, + } + scenario.settings.get_source_overrides.return_value = { + "tool-worker": local.as_uri(), + "hooks-watch": specific, + } + scenario.settings.get_provider_overrides.return_value = [ + {"module": "hooks-watch", "source": provider} + ] + scenario.behind.add(provider) + statuses = asyncio.run(updater._check_all_bundle_status()) + assert local.as_uri() in statuses + assert provider in statuses + assert TOOL not in statuses and HOOK not in statuses and specific not in statuses + scenario.invoke("--yes") + assert scenario.refreshed == [provider] + assert TOOL not in scenario.checked and HOOK not in scenario.checked + + +def test_bundle_include_overrides_use_runtime_resolver_and_aliases(scenario, tmp_path): + local = tmp_path / "override.yaml" + local.write_text( + yaml.safe_dump( + { + "bundle": {"name": "override"}, + "tools": [ + {"module": "tool-extra", "source": MODULES + "#subdirectory=extra"} + ], + } + ), + encoding="utf-8", + ) + scenario.settings.get_bundle_sources.return_value = { + "child": local.as_uri(), + "included.git": local.as_uri(), + } + statuses = asyncio.run(updater._check_all_bundle_status()) + assert MODULES + "#subdirectory=extra" in statuses + assert CHILD not in statuses + assert CHILD not in scenario.checked + + +def test_local_overrides_also_suppress_replaced_legacy_repository_refresh( + scenario, tmp_path +): + local = tmp_path / "local-module" + local.mkdir() + scenario.settings.get_source_overrides.return_value = { + "tool-worker": local.as_uri(), + "hooks-watch": local.as_uri(), + } + scenario.report.cached_git_sources = [ + CachedGitStatus( + name="unrelated-display-name", + url="https://example.invalid/team/modules.git", + ref="feature/update", + has_update=True, + ) + ] + output = scenario.invoke("--yes") + assert scenario.report.cached_git_sources == [] + assert not scenario.refreshed + assert TOOL not in scenario.checked and HOOK not in scenario.checked + assert "All sources up to date" not in output + scenario.apply.assert_not_called() + scenario.execute.assert_not_called() + + +@pytest.mark.parametrize("unreadable", [False, True]) +def test_missing_or_unreadable_cached_manifest_is_explicit_not_green( + scenario, unreadable +): + scenario.behind.clear() + path = scenario.active_path(APP) + if unreadable: + path.write_text("bundle: [broken\n", encoding="utf-8") + else: + path.unlink() + statuses = asyncio.run(updater._check_all_bundle_status()) + assert any(source.error for source in statuses["root"].sources) + assert not statuses["root"].has_updates + output = scenario.invoke("--check-only") + assert "Check failed: incomplete module discovery" in output + assert "could not be checked" in output + assert "All sources up to date" not in output + assert not scenario.refreshed + + +def test_one_status_failure_keeps_other_action_and_hides_private_credentials(scenario): + scenario.errors[TOOL] = TimeoutError( + "https://private-user:private-token@example.invalid/unavailable" + ) + output = scenario.invoke("--check-only", "--verbose") + assert "timeout" in output + assert "private-token" not in output and "private-user" not in output + assert "Update 1 module" in output + assert "Module:" in output + applied = scenario.invoke("--yes") + assert scenario.refreshed == [HOOK] + assert "Update completed with errors" in applied + assert "timeout" in applied and "private-token" not in applied + + +def test_credential_bearing_module_uri_is_an_exact_action_not_a_display_label(scenario): + private_uri = TOOL.replace("https://", "https://fixture-user:fixture-token@") + scenario.write_bundle( + APP, {"tools": [{"module": "tool-worker", "source": private_uri}]} + ) + scenario.behind.clear() + scenario.behind.add(private_uri) + checked = scenario.invoke("--check-only", "--verbose") + assert "fixture-user" not in checked and "fixture-token" not in checked + applied = scenario.invoke("--yes") + assert scenario.refreshed == [private_uri] + assert scenario.apply.await_args.kwargs["selective"] == [private_uri] + assert "fixture-user" not in applied and "fixture-token" not in applied + + +def test_module_action_identity_keeps_dot_git_and_different_refs(scenario): + no_suffix = TOOL.replace("modules.git@", "modules@") + other_ref = TOOL.replace("@feature/update", "@release/next") + sources = [TOOL, no_suffix, other_ref] + scenario.write_bundle( + APP, + { + "tools": [ + {"module": f"tool-{index}", "source": uri} + for index, uri in enumerate(sources) + ] + }, + ) + scenario.behind.clear() + scenario.behind.update(sources) + scenario.invoke("--yes") + assert scenario.refreshed == sources + assert [call.kwargs["selective"] for call in scenario.apply.await_args_list] == [ + [uri] for uri in sources + ] + + +def test_module_install_failure_continues_and_has_honest_terminal_summary(scenario): + async def install(path): + if path == scenario.active_path(TOOL): + raise RuntimeError("authentication private-token rejected") + + scenario.installs.side_effect = install + output = scenario.invoke("--yes") + assert scenario.refreshed == [TOOL, HOOK] + assert "Update completed with errors" in output + assert "Failed to update module" in output + assert "authentication" in output + assert "private-token" not in output + + +@pytest.mark.parametrize("force", [False, True]) +def test_check_only_even_with_force_cannot_clear_resolve_update_or_install( + scenario, force +): + before = { + path.relative_to(scenario.cache): path.read_bytes() + for path in scenario.cache.rglob("*") + if path.is_file() + } + scenario.invoke("--check-only", *(["--force"] if force else [])) + after = { + path.relative_to(scenario.cache): path.read_bytes() + for path in scenario.cache.rglob("*") + if path.is_file() + } + assert after == before + assert not scenario.refreshed + scenario.apply.assert_not_called() + scenario.installs.assert_not_called() + scenario.execute.assert_not_called() + scenario.clear.assert_not_called() + scenario.skills.assert_not_called() + scenario.save.assert_not_called() + scenario.forbidden.assert_not_called() + + +def test_legacy_cache_dedup_uses_url_ref_and_not_basename(scenario): + selected = CachedGitStatus( + name="different-entry-point", + url=MODULES.split("@")[0].removeprefix("git+"), + ref="feature/update", + has_update=True, + ) + other_ref = CachedGitStatus( + name="same-basename", + url=selected.url, + ref="other", + has_update=True, + ) + other_repo = CachedGitStatus( + name="same-basename", + url="https://example.invalid/other/modules.git", + ref="feature/update", + has_update=True, + ) + scenario.report.cached_git_sources = [selected, other_ref, other_repo] + scenario.invoke("--yes") + assert scenario.report.cached_git_sources == [other_ref, other_repo] + assert scenario.execute.await_args.args[0].cached_git_sources == [ + other_ref, + other_repo, + ] + assert scenario.refreshed == [TOOL, HOOK] + + +def test_pinned_module_is_reported_without_action_and_unrelated_source_is_untouched( + scenario, +): + pinned = "git+https://example.invalid/team/pinned.git@v1.2.3#subdirectory=module" + scenario.write_bundle(APP, {"tools": [{"module": "tool-pin", "source": pinned}]}) + scenario.behind.clear() + # Even a checker returning has_update=True cannot select a pinned module. + scenario.behind.add(pinned) + output = scenario.invoke("--check-only") + assert "Pinned" in output + assert "All sources up to date" not in output + scenario.invoke("--yes") + assert not scenario.refreshed + scenario.apply.assert_not_called() + + +def test_root_refresh_selects_only_its_own_uri_not_current_declared_modules( + scenario, monkeypatch +): + scenario.behind.clear() + scenario.behind.add(ROOT) + + async def load_root(uri, *, auto_include): + assert uri == ROOT and auto_include is False + bundle = Bundle(name="root", tools=[{"module": "tool-worker", "source": TOOL}]) + bundle._source_uri = uri + return bundle + + monkeypatch.setattr(amplifier_foundation, "load_bundle", load_root) + scenario.invoke("--yes") + assert scenario.refreshed == [ROOT] + assert scenario.apply.await_args.kwargs == {"selective": [ROOT]} + scenario.installs.assert_not_called() + + +def test_real_discovery_walks_all_same_repo_root_manifests_before_row_dedup( + scenario, monkeypatch +): + second = ROOT + "#subdirectory=behaviors/second.yaml" + unreachable = MODULES + "#subdirectory=unreachable" + scenario.write_bundle(ROOT, {}) + scenario.write_bundle(APP, {"tools": [{"module": "tool-worker", "source": TOOL}]}) + scenario.write_bundle( + second, {"hooks": [{"module": "hooks-watch", "source": HOOK}]} + ) + scenario.write_bundle( + CHILD, {"tools": [{"module": "tool-unreachable", "source": unreachable}]} + ) + scenario.registry.register({"first-root": APP, "second-root": second}) + scenario.registry.save() + persisted = json.loads( + (scenario.cache.parent / "registry.json").read_text(encoding="utf-8") + ) + assert persisted["bundles"]["first-root"]["is_root"] is True + assert persisted["bundles"]["second-root"]["is_root"] is True + assert persisted["bundles"]["child"]["is_root"] is False + scenario.settings.get_app_bundles.return_value = [] + monkeypatch.setattr(discovery_module, "WELL_KNOWN_BUNDLES", {}) + monkeypatch.setattr( + "amplifier_app_cli.lib.settings.AppSettings", lambda: scenario.settings + ) + monkeypatch.setattr( + updater, "AppBundleDiscovery", discovery_module.AppBundleDiscovery + ) + monkeypatch.setattr(updater, "create_bundle_registry", create_bundle_registry) + + discovery = discovery_module.AppBundleDiscovery() + assert discovery.list_cached_root_bundles() == ["root"] + statuses = asyncio.run(updater._check_all_bundle_status()) + assert set(statuses) == {"root", TOOL, HOOK} + assert not statuses["root"].has_updates + assert statuses[TOOL].module_id == "tool-worker" + assert statuses[HOOK].module_id == "hooks-watch" + assert unreachable not in scenario.checked + output = scenario.invoke("--check-only") + assert "Update 2 modules" in output + assert "Update 2 bundles" not in output + scenario.invoke("--yes") + assert scenario.refreshed == [TOOL, HOOK] + assert scenario.apply.await_count == 2 + + +@pytest.mark.parametrize( + ("remote_commit", "expected"), [("b" * 40, "download"), (None, "check_failed")] +) +def test_missing_module_uses_real_foundation_status_without_green_current( + scenario, monkeypatch, remote_commit, expected +): + missing = "git+https://example.invalid/team/absent.git@main" + scenario.write_bundle( + APP, {"tools": [{"module": "tool-absent", "source": missing}]} + ) + scenario.behind.clear() + assert not scenario.active_path(missing).exists() + existing_status = GitSourceHandler.get_status + + async def check_status(self, parsed, cache_dir): + if _uri(parsed) == missing: + return await scenario.real_get_status(self, parsed, cache_dir) + return await existing_status(self, parsed, cache_dir) + + remote = AsyncMock(return_value=remote_commit) + monkeypatch.setattr(GitSourceHandler, "_get_remote_commit", remote) + monkeypatch.setattr(GitSourceHandler, "get_status", check_status) + statuses = asyncio.run(updater._check_all_bundle_status()) + source = statuses[missing].sources[0] + assert source.is_cached is False + assert source.cached_commit is None + expected_update = True if remote_commit else None + assert source.has_update is expected_update + assert updater._classify_source_status(source) == expected + output = scenario.invoke("--check-only") + assert "All sources up to date" not in output + assert "Downloaded module" not in output + if remote_commit: + assert "Download 1 module" in output + else: + assert "could not be checked" in output + remote.assert_awaited_with("https://example.invalid/team/absent.git", "main") + scenario.apply.assert_not_called() + scenario.forbidden.assert_not_called() + + +def test_unknown_missing_module_is_not_current_or_an_update_action( + scenario, monkeypatch +): + missing = "git+https://example.invalid/team/unknown.git@main" + scenario.write_bundle( + APP, {"tools": [{"module": "tool-unknown", "source": missing}]} + ) + existing_status = GitSourceHandler.get_status + + async def check_status(self, parsed, cache_dir): + if _uri(parsed) == missing: + return SourceStatus(source_uri=missing, is_cached=False, has_update=None) + return await existing_status(self, parsed, cache_dir) + + monkeypatch.setattr(GitSourceHandler, "get_status", check_status) + statuses = asyncio.run(updater._check_all_bundle_status()) + assert not statuses[missing].has_updates + assert ( + updater._classify_source_status(statuses[missing].sources[0]) == "not_checked" + ) + output = scenario.invoke("--check-only") + assert "Not checked" in output + assert "could not be checked" in output + assert "All sources up to date" not in output + scenario.apply.assert_not_called() diff --git a/tests/test_transitive_include_updates.py b/tests/test_transitive_include_updates.py index c94e766a..91a97544 100644 --- a/tests/test_transitive_include_updates.py +++ b/tests/test_transitive_include_updates.py @@ -646,7 +646,15 @@ async def test_check_all_bundle_status_includes_transitive_sources( lambda *a, **k: SimpleNamespace(list_cached_root_bundles=lambda: ["b"]), ) monkeypatch.setattr( - update_module, "AppSettings", lambda: SimpleNamespace(get_app_bundles=list) + update_module, + "AppSettings", + lambda: SimpleNamespace( + get_app_bundles=list, + get_bundle_sources=dict, + get_module_sources=dict, + get_source_overrides=dict, + get_provider_overrides=list, + ), ) results = await update_module._check_all_bundle_status() diff --git a/tests/test_update_reporting.py b/tests/test_update_reporting.py index 5c60190f..f58d9d6e 100644 --- a/tests/test_update_reporting.py +++ b/tests/test_update_reporting.py @@ -362,7 +362,8 @@ async def fake_load_bundle(uri, *, auto_include): direct_loads.append(uri) return SimpleNamespace(uri=uri) - async def fake_update_bundle(bundle): + async def fake_update_bundle(bundle, *, selective): + assert selective == [bundle.uri] direct_updates.append(bundle.uri) async def fake_refresh(uri, *, cache_dir): @@ -977,7 +978,8 @@ async def fake_load_bundle(source_uri, *, auto_include): loads.append(source_uri) return SimpleNamespace() - async def fake_update_bundle(bundle): + async def fake_update_bundle(bundle, *, selective): + assert selective == [uri] updates.append("called") patches = _command_patches(report, {"mixed": mixed}) @@ -1059,7 +1061,8 @@ async def fake_load_bundle(uri, *, auto_include): loads.append(uri) return SimpleNamespace() - async def fake_update_bundle(bundle): + async def fake_update_bundle(bundle, *, selective): + assert len(selective) == 1 return None patches = _command_patches(report, bundle_results) @@ -1113,7 +1116,8 @@ async def fake_load_bundle(uri, *, auto_include): raise RuntimeError("cannot load update") return SimpleNamespace(uri=uri) - async def fake_update_bundle(bundle): + async def fake_update_bundle(bundle, *, selective): + assert len(selective) == 1 return None patches = _command_patches(report, bundle_results) From 50170f75eef899b4ad824bb4a4755a7fbd5f257b Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 14 Oct 2026 08:24:34 -0700 Subject: [PATCH 2/2] fix: resolve cached file URIs to native paths Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- AGENTS.md | 1 + amplifier_app_cli/commands/update.py | 46 +--------------- amplifier_app_cli/utils/file_uri.py | 51 ++++++++++++++++++ amplifier_app_cli/utils/include_graph.py | 11 +++- tests/test_declared_module_updates.py | 67 ++++++++++++++++++++++-- 5 files changed, 127 insertions(+), 49 deletions(-) create mode 100644 amplifier_app_cli/utils/file_uri.py diff --git a/AGENTS.md b/AGENTS.md index c81d1967..faad9edf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,6 +110,7 @@ deduplication, then check all five declared module kinds with runtime source override precedence. Preserve exact module URIs (ref, `.git`, subdirectory); refresh/install them through Foundation's selective module update, never load them as bundles. Missing manifests are incomplete discovery, not current. +Convert explicit file URIs to native paths with one decode; keep ordinary paths literal. Keep `--check-only --force` read-only; run `tests/test_declared_module_updates.py`. ## Persisted reminder display diff --git a/amplifier_app_cli/commands/update.py b/amplifier_app_cli/commands/update.py index af2d411b..f3d25bcc 100644 --- a/amplifier_app_cli/commands/update.py +++ b/amplifier_app_cli/commands/update.py @@ -5,7 +5,6 @@ from dataclasses import dataclass from dataclasses import replace import hashlib -import nturl2path import os from pathlib import Path from typing import TYPE_CHECKING, Any, Literal @@ -31,6 +30,7 @@ from ..utils.atomic_write import atomic_write_json from ..utils.display import create_sha_text from ..utils.error_format import escape_markup +from ..utils.file_uri import file_uri_path_value as _file_uri_path_value from ..utils.settings_manager import save_update_last_check from ..utils.source_status import check_all_sources from ..utils.update_executor import execute_updates @@ -149,50 +149,6 @@ def _is_valid_app_source(uri: str) -> bool: return False -def _file_uri_path_value(uri: str, *, windows: bool) -> str | None: - """Return a local filename from a file URI for the named platform. - - ``Path.as_uri()`` emits ``file:///C:/...`` on Windows, while the legacy - CLI interpolation emits ``file://C:\\...``. Both forms name a local drive - there. A named host is a UNC path only on Windows; on POSIX it is remote - and deliberately not reinterpreted as a local path. - """ - - parsed = _safe_urlsplit(uri) - if parsed is None or parsed.scheme.lower() != "file": - return None - - netloc = parsed.netloc - if not netloc or netloc.lower() == "localhost": - encoded_path = parsed.path - elif windows and len(netloc) >= 2 and netloc[1] == ":" and netloc[0].isalpha(): - # Legacy ``file://C:\path`` serialization. ``nturl2path`` accepts - # both backslash and slash separators after the drive. - encoded_path = netloc + parsed.path - elif windows and "@" not in netloc and ":" not in netloc: - # Standard UNC syntax: file://server/share/path -> \\server\share\path. - try: - hostname = parsed.hostname - except ValueError: - return None - if not hostname or hostname.lower() != netloc.lower(): - return None - if not parsed.path.strip("/"): - return None - encoded_path = f"//{hostname}{parsed.path}" - else: - return None - - # urllib.request.url2pathname follows the host OS. Use percent-decoding - # explicitly for POSIX so the requested platform, not the host, wins. - converter = nturl2path.url2pathname if windows else unquote - try: - path_value = converter(encoded_path) - except (OSError, ValueError): - return None - return path_value if path_value and "\x00" not in path_value else None - - def _file_uri_path(uri: str) -> Path | None: """Return the local path represented by a file URI, without its fragment.""" diff --git a/amplifier_app_cli/utils/file_uri.py b/amplifier_app_cli/utils/file_uri.py new file mode 100644 index 00000000..eded70e7 --- /dev/null +++ b/amplifier_app_cli/utils/file_uri.py @@ -0,0 +1,51 @@ +"""Convert explicit file URIs to native paths without filesystem access.""" + +import nturl2path +from urllib.parse import unquote, urlsplit + + +def file_uri_path_value(uri: str, *, windows: bool) -> str | None: + """Return a local filename from a file URI for the named platform. + + ``Path.as_uri()`` emits ``file:///C:/...`` on Windows, while the legacy + CLI interpolation emits ``file://C:\\...``. Both forms name a local drive + there. A named host is a UNC path only on Windows; on POSIX it is remote + and deliberately not reinterpreted as a local path. + """ + + try: + parsed = urlsplit(uri) + except ValueError: + return None + if parsed.scheme.lower() != "file": + return None + + netloc = parsed.netloc + if not netloc or netloc.lower() == "localhost": + encoded_path = parsed.path + elif windows and len(netloc) >= 2 and netloc[1] == ":" and netloc[0].isalpha(): + # Legacy ``file://C:\path`` serialization. ``nturl2path`` accepts + # both backslash and slash separators after the drive. + encoded_path = netloc + parsed.path + elif windows and "@" not in netloc and ":" not in netloc: + # Standard UNC syntax: file://server/share/path -> \\server\share\path. + try: + hostname = parsed.hostname + except ValueError: + return None + if not hostname or hostname.lower() != netloc.lower(): + return None + if not parsed.path.strip("/"): + return None + encoded_path = f"//{hostname}{parsed.path}" + else: + return None + + # urllib.request.url2pathname follows the host OS. Use percent-decoding + # explicitly for POSIX so the requested platform, not the host, wins. + converter = nturl2path.url2pathname if windows else unquote + try: + path_value = converter(encoded_path) + except (OSError, ValueError): + return None + return path_value if path_value and "\x00" not in path_value else None \ No newline at end of file diff --git a/amplifier_app_cli/utils/include_graph.py b/amplifier_app_cli/utils/include_graph.py index 8e75e74c..03b5018c 100644 --- a/amplifier_app_cli/utils/include_graph.py +++ b/amplifier_app_cli/utils/include_graph.py @@ -39,6 +39,7 @@ from __future__ import annotations import logging +import os from collections.abc import AsyncIterator from dataclasses import dataclass from dataclasses import replace @@ -46,6 +47,8 @@ from typing import TYPE_CHECKING from typing import Mapping +from .file_uri import file_uri_path_value + if TYPE_CHECKING: from amplifier_foundation import Bundle, BundleRegistry from amplifier_foundation.sources.protocol import SourceStatus @@ -145,7 +148,13 @@ def _local_path_for(uri: str, cache_dir: Path) -> Path | None: return active if active.exists() else None if parsed.is_file: - path = Path(parsed.path) + if uri.lower().startswith("file:"): + native = file_uri_path_value(uri, windows=os.name == "nt") + if native is None: + return None + path = Path(native) + else: + path = Path(parsed.path) if parsed.subpath: path = path / parsed.subpath return path if path.exists() else None diff --git a/tests/test_declared_module_updates.py b/tests/test_declared_module_updates.py index 3cad2ac3..5249e428 100644 --- a/tests/test_declared_module_updates.py +++ b/tests/test_declared_module_updates.py @@ -23,7 +23,11 @@ from amplifier_app_cli.lib.bundle_loader import discovery as discovery_module from amplifier_app_cli.lib.settings import AppSettings from amplifier_app_cli.paths import create_bundle_registry -from amplifier_app_cli.utils.include_graph import collect_declared_module_sources +from amplifier_app_cli.utils.file_uri import file_uri_path_value +from amplifier_app_cli.utils.include_graph import ( + _local_path_for, + collect_declared_module_sources, +) from amplifier_app_cli.utils.source_status import CachedGitStatus, UpdateReport from amplifier_app_cli.utils.update_executor import ExecutionResult @@ -317,8 +321,11 @@ def test_module_override_precedence_and_local_source_never_refresh_original( assert TOOL not in scenario.checked and HOOK not in scenario.checked -def test_bundle_include_overrides_use_runtime_resolver_and_aliases(scenario, tmp_path): - local = tmp_path / "override.yaml" +@pytest.mark.parametrize("filename", ["override.yaml", "override space %20.yaml"]) +def test_bundle_include_overrides_use_runtime_resolver_and_aliases( + scenario, tmp_path, filename +): + local = tmp_path / filename local.write_text( yaml.safe_dump( { @@ -655,3 +662,57 @@ async def check_status(self, parsed, cache_dir): assert "could not be checked" in output assert "All sources up to date" not in output scenario.apply.assert_not_called() + + +def test_cached_bundle_include_file_uri_round_trip_decodes_once(scenario, tmp_path): + local = tmp_path / "manifest space %20.yaml" + extra = MODULES + "#subdirectory=extra" + local.write_text( + yaml.safe_dump( + { + "bundle": {"name": "local-manifest"}, + "tools": [{"module": "tool-extra", "source": extra}], + } + ), + encoding="utf-8", + ) + uri = local.as_uri() + assert _local_path_for(uri, scenario.cache) == local + scenario.write_bundle(CHILD, {"includes": [{"bundle": uri}]}) + + found, failures = asyncio.run( + collect_declared_module_sources( + {"root": ROOT}, + registry=scenario.registry, + cache_dir=scenario.cache, + ) + ) + assert not failures + assert set(found) == {extra} + assert found[extra].declared_by == "local-manifest" + scenario.forbidden.assert_not_called() + + +def test_ordinary_manifest_path_preserves_literal_percent_escape(tmp_path): + local = tmp_path / "manifest space %20.yaml" + local.write_text("bundle: {name: literal-path}\n", encoding="utf-8") + + assert _local_path_for(str(local), tmp_path / "cache") == local + + +@pytest.mark.parametrize( + ("uri", "expected"), + [ + ( + "file:///C:/space%20name/manifest.yaml", + r"C:\space name\manifest.yaml", + ), + ( + "file://localhost/C:/space%20name/manifest.yaml", + r"C:\space name\manifest.yaml", + ), + ("file://server/share/space%20name", r"\\server\share\space name"), + ], +) +def test_shared_file_uri_conversion_uses_explicit_windows_platform(uri, expected): + assert file_uri_path_value(uri, windows=True) == expected