From f0b62a401a1683da4ed99ea99d3618fa872a120e Mon Sep 17 00:00:00 2001 From: xuyuanhao Date: Fri, 1 May 2026 22:50:59 +0800 Subject: [PATCH 1/5] fix(deps): support meta-packages and apm.yml inside collections/ subpaths apm install rejected meta-package layouts in two ways: paths containing /collections/ hard-routed to the .collection.yml parser even when an apm.yml lived inside, and a dependency-only apm.yml without a .apm/ directory failed validation. Both stem from the same architectural error: package type was inferred from path segments rather than content. Drop the /collections/ path heuristic in virtual_type in favor of extension-only classification (.collection.yml signals COLLECTION; everything else is SUBDIRECTORY, resolved at fetch time). Probe order in validate_virtual_package_exists puts apm.yml before the legacy .collection.yml fallback so a fixed-but-mis-routed collections// apm.yml is recognised correctly. Recognise META_PACKAGE as a first-class PackageType so dependency-only aggregators no longer need an empty .apm/.gitkeep placeholder. Legacy /collections/ URLs that resolve to a sibling .collection.yml still work via a content-only fallback in the dispatcher. Fixes #1094 --- src/apm_cli/deps/github_downloader.py | 66 ++++++- .../deps/github_downloader_validation.py | 14 +- src/apm_cli/install/sources.py | 1 + src/apm_cli/models/dependency/reference.py | 77 ++++---- src/apm_cli/models/validation.py | 91 ++++++++- tests/integration/test_collection_install.py | 27 ++- tests/test_apm_package_models.py | 38 +++- .../deps/test_github_downloader_validation.py | 183 ++++++++++++++++++ tests/unit/test_ado_path_structure.py | 11 +- tests/unit/test_artifactory_support.py | 3 +- tests/unit/test_generic_git_urls.py | 6 +- tests/unit/test_meta_package.py | 162 ++++++++++++++++ tests/unit/test_package_identity.py | 55 ++++-- tests/unit/test_script_runner.py | 5 +- 14 files changed, 666 insertions(+), 73 deletions(-) create mode 100644 tests/unit/test_meta_package.py diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index a1aaa5ef6..5deb8b56c 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1449,7 +1449,12 @@ def download_collection_package( if not dep_ref.is_virtual or not dep_ref.virtual_path: raise ValueError("Dependency must be a virtual collection package") - if not dep_ref.is_virtual_collection(): + # Accept both COLLECTION (explicit `.collection.yml` URL) and + # SUBDIRECTORY (legacy `/collections/` heuristic resolved at + # fetch time per #1094). The dispatcher in `download_package` + # decides which downloader to invoke; here we only reject + # non-virtual or shape-incompatible references. + if not (dep_ref.is_virtual_collection() or dep_ref.is_virtual_subdirectory()): raise ValueError(f"Path '{dep_ref.virtual_path}' is not a valid collection path") # Determine the ref to use @@ -1651,6 +1656,55 @@ def _try_sparse_checkout( _debug(f"Sparse-checkout failed: {e}") return False + def _is_legacy_collection_fallback(self, dep_ref: DependencyReference) -> bool: + """Return True iff this SUBDIRECTORY ref is a legacy collection ref. + + A legacy collection reference is a path like ``collections/`` + with NO ``apm.yml`` at the path but WITH a sibling + ``.collection.yml`` (or ``.yaml``) at the parent. Predates the + extension-based URL form ``collections/.collection.yml``. + + Resolution order matches `validate_virtual_package_exists` and the + issue #1094 acceptance criterion: ``apm.yml`` always wins; the + collection manifest is the documented fallback. The ``apm.yml`` + probe is intentionally first so it short-circuits the (cheap) but + still-unnecessary collection probes for the common case. + + Performance: the probes only run for paths that match the legacy + ``/collections/`` convention -- a path-shape *hint*, not a + classification. The actual shape is still confirmed by network + probes; this only narrows the search space so SUBDIRECTORY refs + outside the ``/collections/`` convention (skill bundles, plugin + repos, etc.) skip 2-3 wasted HTTP calls per install. + """ + if not dep_ref.is_virtual_subdirectory(): + return False + vpath = dep_ref.virtual_path + # Perf hint: legacy `.collection.yml` files conventionally live + # under a `collections/` segment. Skip the probes for paths that + # don't match this shape -- the dispatcher will fall through to + # the regular subdirectory clone path with no extra HTTP cost. + if "/collections/" not in vpath and not vpath.startswith("collections/"): + return False + ref = dep_ref.reference or "main" + # If `/apm.yml` exists, this is a real APM/meta package, not a + # legacy collection reference. Short-circuit immediately so we never + # mis-route a fixed-but-similarly-shaped layout to the collection + # downloader. + try: + self.download_raw_file(dep_ref, f"{vpath}/apm.yml", ref) + return False + except RuntimeError: + pass + # Probe sibling collection manifests in priority order. + for ext in (".collection.yml", ".collection.yaml"): + try: + self.download_raw_file(dep_ref, f"{vpath}{ext}", ref) + return True + except RuntimeError: + continue + return False + def download_subdirectory_package( self, dep_ref: DependencyReference, @@ -2103,6 +2157,16 @@ def download_package( return self._download_subdirectory_from_artifactory( dep_ref, target_path, art_proxy, progress_task_id, progress_obj ) + # Legacy collection fallback (#1094): a SUBDIRECTORY reference + # whose path has no `apm.yml` but whose sibling + # `.collection.yml` exists is a legacy collection + # reference (predates the extension-based URL form). Route to + # the collection downloader instead of cloning a directory + # that does not exist. + if self._is_legacy_collection_fallback(dep_ref): + return self.download_collection_package( + dep_ref, target_path, progress_task_id, progress_obj + ) return self.download_subdirectory_package( dep_ref, target_path, progress_task_id, progress_obj ) diff --git a/src/apm_cli/deps/github_downloader_validation.py b/src/apm_cli/deps/github_downloader_validation.py index 4d9ac1a5a..51fd4e626 100644 --- a/src/apm_cli/deps/github_downloader_validation.py +++ b/src/apm_cli/deps/github_downloader_validation.py @@ -156,12 +156,22 @@ def _probe(path: str) -> bool: _log(f' [i] Validating virtual package at ref "{ref}": {dep_ref.repo_url}/{vpath}') if dep_ref.is_virtual_collection(): - return _probe(f"{vpath}.collection.yml") + # COLLECTION refs always have an explicit `.collection.yml` / + # `.collection.yaml` extension at the end of vpath (per the + # extension-only classification in `virtual_type`). Probe the file + # directly -- appending another `.collection.yml` would produce a + # non-existent double-extension path. + return _probe(vpath) if dep_ref.is_virtual_file(): return _probe(vpath) if dep_ref.is_virtual_subdirectory(): + # Probe order is intentional: apm.yml takes priority over the legacy + # `.collection.yml` fallback so a `collections//apm.yml` + # is recognised as an APM package, not as a missing collection + # manifest (microsoft/apm#1094). Sibling .collection.yml files at the + # parent path remain a recognised legacy form. marker_paths = [ f"{vpath}/apm.yml", f"{vpath}/SKILL.md", @@ -169,6 +179,8 @@ def _probe(path: str) -> bool: f"{vpath}/.github/plugin/plugin.json", f"{vpath}/.claude-plugin/plugin.json", f"{vpath}/.cursor-plugin/plugin.json", + f"{vpath}.collection.yml", + f"{vpath}.collection.yaml", f"{vpath}/README.md", ] for marker_path in marker_paths: diff --git a/src/apm_cli/install/sources.py b/src/apm_cli/install/sources.py index 35bf4fe9d..e62dafc37 100644 --- a/src/apm_cli/install/sources.py +++ b/src/apm_cli/install/sources.py @@ -58,6 +58,7 @@ def _format_package_type_label(pkg_type) -> str | None: PackageType.APM_PACKAGE: "APM Package (apm.yml)", PackageType.HOOK_PACKAGE: "Hook Package (hooks/*.json only)", PackageType.SKILL_BUNDLE: "Skill Bundle (skills//SKILL.md)", + PackageType.META_PACKAGE: "Meta Package (apm.yml dependency aggregator)", }.get(pkg_type) diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index 9b2c0433e..8ec8781e8 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -65,6 +65,16 @@ class DependencyReference: ".agent.md", ) + # Explicit collection-manifest extensions. A virtual_path that ends in one + # of these is unambiguously a `.collection.yml` reference. Paths that + # merely contain a `collections/` segment are NOT classified here -- they + # are SUBDIRECTORY references whose actual shape is resolved at fetch + # time, with `.collection.yml` as a documented fallback (see #1094). + VIRTUAL_COLLECTION_EXTENSIONS = ( + ".collection.yml", + ".collection.yaml", + ) + def is_artifactory(self) -> bool: """Check if this reference points to a JFrog Artifactory VCS repository.""" return self.artifactory_prefix is not None @@ -77,12 +87,19 @@ def is_azure_devops(self) -> bool: @property def virtual_type(self) -> "VirtualPackageType | None": - """Return the type of virtual package, or None if not virtual.""" + """Return the type of virtual package, or None if not virtual. + + Classification is by extension only -- never by path segment. A path + like ``collections/foo`` is SUBDIRECTORY, NOT COLLECTION; the actual + shape is resolved at fetch time by probing for ``apm.yml`` first and + falling back to ``foo.collection.yml`` (see #1094). COLLECTION is + reserved for URLs that explicitly name a ``.collection.yml`` file. + """ if not self.is_virtual or not self.virtual_path: return None if any(self.virtual_path.endswith(ext) for ext in self.VIRTUAL_FILE_EXTENSIONS): return VirtualPackageType.FILE - if "/collections/" in self.virtual_path or self.virtual_path.startswith("collections/"): + if any(self.virtual_path.endswith(ext) for ext in self.VIRTUAL_COLLECTION_EXTENSIONS): return VirtualPackageType.COLLECTION return VirtualPackageType.SUBDIRECTORY @@ -97,15 +114,18 @@ def is_virtual_collection(self) -> bool: def is_virtual_subdirectory(self) -> bool: """Check if this is a virtual subdirectory package (e.g., Claude Skill). - A subdirectory package is a virtual package that: - - Has a virtual_path that is NOT a file extension we recognize - - Is NOT a collection (doesn't have /collections/ in path) - - Is a directory path (likely containing SKILL.md or apm.yml) + A subdirectory package is a virtual package whose ``virtual_path`` + does not end in a recognized FILE or COLLECTION extension. The + actual on-disk shape is resolved at fetch time -- ``apm.yml``, + ``SKILL.md``, ``plugin.json``, or a sibling ``.collection.yml`` + (legacy fallback for ``/collections/`` paths, see #1094). Examples: - ComposioHQ/awesome-claude-skills/brand-guidelines -> True - owner/repo/prompts/file.prompt.md -> False (is_virtual_file) - - owner/repo/collections/name -> False (is_virtual_collection) + - owner/repo/collections/name -> True (resolved at fetch time) + - owner/repo/collections/name.collection.yml -> False + (is_virtual_collection) """ return self.virtual_type == VirtualPackageType.SUBDIRECTORY @@ -126,26 +146,15 @@ def get_virtual_package_name(self) -> str: # Get the basename without extension path_parts = self.virtual_path.split("/") - if self.is_virtual_collection(): - # For collections: use the collection name without extension - # collections/project-planning -> project-planning - # collections/project-planning.collection.yml -> project-planning - collection_name = path_parts[-1] - # Strip .collection.yml/.collection.yaml extension if present - for ext in (".collection.yml", ".collection.yaml"): - if collection_name.endswith(ext): - collection_name = collection_name[: -len(ext)] - break - return f"{repo_name}-{collection_name}" - else: - # For individual files: use the filename without extension - # prompts/code-review.prompt.md -> code-review - filename = path_parts[-1] - for ext in self.VIRTUAL_FILE_EXTENSIONS: - if filename.endswith(ext): - filename = filename[: -len(ext)] - break - return f"{repo_name}-{filename}" + last = path_parts[-1] + # Strip any recognised virtual extension (file, collection, or none). + # Same naming rule applies regardless of resolved shape: the directory + # name (or file basename) is the user-visible package name. + for ext in self.VIRTUAL_COLLECTION_EXTENSIONS + self.VIRTUAL_FILE_EXTENSIONS: + if last.endswith(ext): + last = last[: -len(ext)] + break + return f"{repo_name}-{last}" @staticmethod def is_local_path(dep_str: str) -> bool: @@ -636,11 +645,12 @@ def _detect_virtual_package(cls, dependency_str: str): # Security: reject path traversal in virtual path validate_path_segments(virtual_path, context="virtual path") - if ( - "/collections/" in check_str - or virtual_path.startswith("collections/") - or any(virtual_path.endswith(ext) for ext in cls.VIRTUAL_FILE_EXTENSIONS) - ): + # Accept any path ending in a recognised virtual extension + # (file or collection-manifest). Reject other dotted final + # segments so typos like `prompts/file.txt` fail fast instead + # of silently mis-classifying as a subdirectory. + recognised_exts = cls.VIRTUAL_FILE_EXTENSIONS + cls.VIRTUAL_COLLECTION_EXTENSIONS + if any(virtual_path.endswith(ext) for ext in recognised_exts): pass else: last_segment = virtual_path.split("/")[-1] @@ -1020,7 +1030,8 @@ def parse(cls, dependency_str: str) -> "DependencyReference": - user/repo@alias - user/repo#ref@alias - user/repo/path/to/file.prompt.md (virtual file package) - - user/repo/collections/name (virtual collection package) + - user/repo/collections/name.collection.yml (virtual collection package) + - user/repo/skills/foo (virtual subdirectory package) - https://gitlab.com/owner/repo.git (generic HTTPS git URL) - git@gitlab.com:owner/repo.git (SSH git URL) - ssh://git@gitlab.com/owner/repo.git (SSH protocol URL) diff --git a/src/apm_cli/models/validation.py b/src/apm_cli/models/validation.py index 2de0bae3a..2c0b9162e 100644 --- a/src/apm_cli/models/validation.py +++ b/src/apm_cli/models/validation.py @@ -27,6 +27,9 @@ class PackageType(Enum): HYBRID = "hybrid" # Has both apm.yml and SKILL.md (root) MARKETPLACE_PLUGIN = "marketplace_plugin" # Has plugin.json or .claude-plugin/ SKILL_BUNDLE = "skill_bundle" # Has skills//SKILL.md (nested), apm.yml optional + META_PACKAGE = ( + "meta_package" # apm.yml declares deps but no .apm/ -- a curated dep aggregator (#1094) + ) INVALID = "invalid" # None of the above @@ -233,10 +236,14 @@ def detect_package_type( 4. ``SKILL_BUNDLE`` -- nested ``skills//SKILL.md`` detected; ``apm.yml`` optional; no ``.apm/`` required. 5. ``APM_PACKAGE`` -- ``apm.yml`` AND ``.apm/`` directory present. - 6. ``INVALID`` -- ``apm.yml`` present but no ``.apm/`` and no nested - skills (helpful error: author likely needs to add .apm/). - 7. ``HOOK_PACKAGE`` -- ``hooks/*.json`` only, no other signals. - 8. ``INVALID`` -- nothing recognisable. + 6. ``META_PACKAGE`` -- ``apm.yml`` only, no ``.apm/`` and no nested + skills, but ``apm.yml`` declares dependencies. The package is a + curated aggregator that contributes no own primitives (#1094). + 7. ``INVALID`` -- ``apm.yml`` present but no ``.apm/``, no nested + skills, AND no declared dependencies (helpful error: author + likely needs to add .apm/ or dependencies). + 8. ``HOOK_PACKAGE`` -- ``hooks/*.json`` only, no other signals. + 9. ``INVALID`` -- nothing recognisable. Returns: A ``(package_type, plugin_json_path)`` tuple. *plugin_json_path* @@ -266,17 +273,53 @@ def detect_package_type( apm_dir = package_path / APM_DIR if apm_dir.is_dir(): return PackageType.APM_PACKAGE, None - # 6. apm.yml only (no .apm/, no nested skills) -> INVALID + # 6. apm.yml + dependencies (no .apm/, no nested skills) -> META_PACKAGE. + # A meta-package is a curated dep aggregator with no own primitives; + # validation passes through to dependency resolution unchanged (#1094). + if _apm_yml_declares_dependencies(package_path / APM_YML_FILENAME): + return PackageType.META_PACKAGE, None + # 7. apm.yml only, nothing useful in it -> INVALID (the original + # error wording still applies: author likely needs `.apm/` or deps). return PackageType.INVALID, None - # 7. hooks/*.json only -> HOOK_PACKAGE + # 8. hooks/*.json only -> HOOK_PACKAGE if evidence.has_hook_json: return PackageType.HOOK_PACKAGE, None - # 8. Nothing recognisable -> INVALID + # 9. Nothing recognisable -> INVALID return PackageType.INVALID, None +def _apm_yml_declares_dependencies(apm_yml_path: Path) -> bool: + """Return True iff ``apm.yml`` declares at least one dependency. + + Used to distinguish a META_PACKAGE (intentional curated aggregator) + from an unfinished APM_PACKAGE (apm.yml authored but `.apm/` not yet + added). Any non-empty ``apm`` or ``mcp`` list under ``dependencies`` + OR ``devDependencies`` qualifies. Tolerant of malformed YAML / + missing keys: returns False on any parse problem so callers get the + legacy INVALID diagnostic rather than a silent reclassification + (#1094). + """ + try: + from ..utils.yaml_io import load_yaml + + data = load_yaml(apm_yml_path) or {} + except Exception: + return False + if not isinstance(data, dict): + return False + + def _has_listed_deps(block: object) -> bool: + if not isinstance(block, dict): + return False + return bool(block.get("apm") or block.get("mcp")) + + return _has_listed_deps(data.get("dependencies")) or _has_listed_deps( + data.get("devDependencies") + ) + + def validate_apm_package(package_path: Path) -> ValidationResult: """Validate that a directory contains a valid APM package or Claude Skill. @@ -355,6 +398,12 @@ def validate_apm_package(package_path: Path) -> ValidationResult: # Standard APM package or HYBRID validation (has apm.yml) apm_yml_path = package_path / APM_YML_FILENAME + # Meta-packages (#1094): apm.yml is purely a curated dep aggregator + # with no `.apm/` and no own primitives. Validation parses apm.yml and + # returns; transitive resolution is handled downstream. + if result.package_type == PackageType.META_PACKAGE: + return _validate_meta_package(package_path, apm_yml_path, result) + # HYBRID packages: if .apm/ exists, fall through to standard validation # (back-compat for packages that ship both .apm/ primitives AND SKILL.md). # Otherwise validate as a skill bundle with apm.yml metadata. @@ -556,6 +605,34 @@ def _validate_skill_bundle(package_path: Path, result: ValidationResult) -> Vali return result +def _validate_meta_package( + package_path: Path, apm_yml_path: Path, result: ValidationResult +) -> ValidationResult: + """Validate a META_PACKAGE (apm.yml dep aggregator, no `.apm/`). + + A meta-package is a curated dependency set: ``apm.yml`` declares + ``dependencies.apm`` and/or ``dependencies.mcp`` and contributes no + own primitives. There is nothing to integrate at the meta-package + level -- transitive resolution and integration happen on the + declared dependencies as usual (#1094). + + Validation parses ``apm.yml`` to confirm it is structurally well-formed + so downstream resolution has a usable APMPackage handle. ``.apm/`` + intentionally not required: requiring an empty placeholder directory + is the wart this type was added to remove. + """ + from .apm_package import APMPackage + + try: + package = APMPackage.from_apm_yml(apm_yml_path) + except (ValueError, FileNotFoundError) as e: + result.add_error(f"Invalid apm.yml: {e}") + return result + + result.package = package + return result + + def _validate_hybrid_package( package_path: Path, apm_yml_path: Path, result: ValidationResult ) -> ValidationResult: diff --git a/tests/integration/test_collection_install.py b/tests/integration/test_collection_install.py index 106b0a226..127da4399 100644 --- a/tests/integration/test_collection_install.py +++ b/tests/integration/test_collection_install.py @@ -14,24 +14,39 @@ class TestCollectionInstallation: """Test collection virtual package installation from GitHub.""" def test_parse_collection_dependency(self): - """Test parsing a collection dependency reference.""" - dep_ref = DependencyReference.parse("owner/test-repo/collections/awesome-copilot") + """Test parsing a collection dependency reference. + Per #1094, `/collections/` (no extension) is now SUBDIRECTORY; + the legacy collection shape is resolved at fetch time via the + `.collection.yml` fallback probe. Explicit-extension URLs + remain classified as COLLECTION up-front. + """ + # Implicit form: classified as SUBDIRECTORY, resolved at fetch time. + dep_ref = DependencyReference.parse("owner/test-repo/collections/awesome-copilot") assert dep_ref.is_virtual is True - assert dep_ref.is_virtual_collection() is True + assert dep_ref.is_virtual_subdirectory() is True + assert dep_ref.is_virtual_collection() is False assert dep_ref.is_virtual_file() is False assert dep_ref.repo_url == "owner/test-repo" assert dep_ref.virtual_path == "collections/awesome-copilot" assert dep_ref.get_virtual_package_name() == "test-repo-awesome-copilot" + # Explicit form: classified as COLLECTION up-front. + dep_ref_explicit = DependencyReference.parse( + "owner/test-repo/collections/awesome-copilot.collection.yml" + ) + assert dep_ref_explicit.is_virtual_collection() is True + assert dep_ref_explicit.get_virtual_package_name() == "test-repo-awesome-copilot" + def test_parse_collection_with_reference(self): """Test parsing a collection dependency with git reference.""" - dep_ref = DependencyReference.parse("owner/test-repo/collections/project-planning#main") - + dep_ref = DependencyReference.parse( + "owner/test-repo/collections/project-planning.collection.yml#main" + ) assert dep_ref.is_virtual is True assert dep_ref.is_virtual_collection() is True assert dep_ref.reference == "main" - assert dep_ref.virtual_path == "collections/project-planning" + assert dep_ref.virtual_path == "collections/project-planning.collection.yml" @pytest.mark.integration @pytest.mark.slow diff --git a/tests/test_apm_package_models.py b/tests/test_apm_package_models.py index c233566b9..a3069667d 100644 --- a/tests/test_apm_package_models.py +++ b/tests/test_apm_package_models.py @@ -374,22 +374,48 @@ def test_parse_virtual_file_all_extensions(self): assert dep.is_virtual_file() is True assert dep.virtual_path == f"path/to/file{ext}" - def test_parse_virtual_collection(self): - """Test parsing virtual collection package.""" + def test_parse_virtual_collection_explicit_extension(self): + """Test parsing virtual collection with explicit `.collection.yml`. + + Per #1094, the legacy `/collections/` path heuristic no + longer eagerly classifies as COLLECTION; the actual shape is + resolved at fetch time. Explicit `.collection.yml` URLs remain + classified as COLLECTION up-front. + """ + dep = DependencyReference.parse( + "owner/test-repo/collections/project-planning.collection.yml" + ) + assert dep.repo_url == "owner/test-repo" + assert dep.is_virtual is True + assert dep.virtual_path == "collections/project-planning.collection.yml" + assert dep.is_virtual_file() is False + assert dep.is_virtual_collection() is True + assert dep.get_virtual_package_name() == "test-repo-project-planning" + + def test_parse_collections_path_resolves_at_fetch_time(self): + """A `/collections/` URL is SUBDIRECTORY now (#1094). + + The actual shape (APM package vs legacy collection manifest) is + resolved at fetch time by probing `apm.yml` first, then + `.collection.yml` as a fallback. + """ dep = DependencyReference.parse("owner/test-repo/collections/project-planning") assert dep.repo_url == "owner/test-repo" assert dep.is_virtual is True assert dep.virtual_path == "collections/project-planning" assert dep.is_virtual_file() is False - assert dep.is_virtual_collection() is True + assert dep.is_virtual_collection() is False + assert dep.is_virtual_subdirectory() is True + # Naming preserved across the reclassification: same package name + # whether the URL has the explicit extension or not. assert dep.get_virtual_package_name() == "test-repo-project-planning" def test_parse_virtual_collection_with_reference(self): - """Test parsing virtual collection with git reference.""" - dep = DependencyReference.parse("owner/test-repo/collections/testing#main") + """Explicit `.collection.yml` reference still parses as COLLECTION.""" + dep = DependencyReference.parse("owner/test-repo/collections/testing.collection.yml#main") assert dep.repo_url == "owner/test-repo" assert dep.is_virtual is True - assert dep.virtual_path == "collections/testing" + assert dep.virtual_path == "collections/testing.collection.yml" assert dep.reference == "main" assert dep.is_virtual_collection() is True diff --git a/tests/unit/deps/test_github_downloader_validation.py b/tests/unit/deps/test_github_downloader_validation.py index 4ec11bf3b..d9f08165c 100644 --- a/tests/unit/deps/test_github_downloader_validation.py +++ b/tests/unit/deps/test_github_downloader_validation.py @@ -686,3 +686,186 @@ def test_explicit_ref_still_activates_git_fallback(self) -> None: assert ok is True ls_remote.assert_called_once() ls_tree.assert_called_once() + + +# --------------------------------------------------------------------------- +# Issue #1094: probe order in SUBDIRECTORY case +# --------------------------------------------------------------------------- + + +class TestSubdirectoryProbeOrder: + """``apm.yml`` is probed before ``.collection.yml`` for SUBDIRECTORY paths. + + The legacy ``/collections/`` path heuristic is removed: a path like + ``collections/foo`` is now classified SUBDIRECTORY and resolved at fetch + time. ``apm.yml`` must win over the ``.collection.yml`` fallback so a + fixed-but-mis-routed ``collections/foo/apm.yml`` is recognised as an + APM package, not as a missing collection manifest (#1094). + """ + + def test_explicit_collection_yml_url_probes_path_directly(self) -> None: + """Explicit `.collection.yml` URLs probe the file directly, no double extension. + + Regression: an earlier implementation appended `.collection.yml` to + every COLLECTION-typed vpath, producing + ``foo.collection.yml.collection.yml`` for explicit-extension URLs. + After the extension-only classification (#1094), COLLECTION refs + already carry the extension in vpath -- the probe must use vpath + directly. + """ + downloader = GitHubPackageDownloader() + # Construct an explicit-extension COLLECTION ref via parser so the + # test exercises the full classification cascade. + from apm_cli.models.apm_package import DependencyReference + + dep_ref = DependencyReference.parse("owner/repo/collections/foo.collection.yml#main") + assert dep_ref.is_virtual_collection() is True + + with patch.object(downloader, "download_raw_file") as raw: + raw.return_value = b"id: foo\nname: foo\ndescription: x\nitems: []\n" + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is True + attempted = [call.args[1] for call in raw.call_args_list] + assert "collections/foo.collection.yml" in attempted + # No double extension. + assert "collections/foo.collection.yml.collection.yml" not in attempted + + def test_apm_yml_at_collections_path_short_circuits_collection_probe(self) -> None: + """`/apm.yml` hits first; `.collection.yml` never probed.""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="collections/writing", ref="main") + + # Whitelist only `/apm.yml`; everything else 404s. + def fake_download(_dr, path, _ref): + if path == "collections/writing/apm.yml": + return b"name: writing\nversion: 1.0.0\n" + raise RuntimeError("404") + + with patch.object(downloader, "download_raw_file", side_effect=fake_download) as raw: + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is True + # Only one HTTP probe should have happened: the apm.yml hit short + # circuits the rest of the probe order. Defensively verify the + # collection probes were never reached. + attempted_paths = [call.args[1] for call in raw.call_args_list] + assert "collections/writing/apm.yml" in attempted_paths + assert "collections/writing.collection.yml" not in attempted_paths + assert "collections/writing.collection.yaml" not in attempted_paths + + def test_collection_yml_fallback_when_no_apm_yml(self) -> None: + """Legacy `.collection.yml` fallback validates when apm.yml absent.""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="collections/legacy", ref="main") + + def fake_download(_dr, path, _ref): + if path == "collections/legacy.collection.yml": + return b"description: legacy collection\nitems: []\n" + raise RuntimeError("404") + + with ( + patch.object(downloader, "download_raw_file", side_effect=fake_download), + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object(gdv, "_ref_exists_via_ls_remote", return_value=(False, None)), + ): + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is True + + def test_collection_yaml_fallback_when_no_apm_yml(self) -> None: + """The `.collection.yaml` extension is also recognised as a fallback.""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="collections/legacy", ref="main") + + def fake_download(_dr, path, _ref): + if path == "collections/legacy.collection.yaml": + return b"description: legacy collection\nitems: []\n" + raise RuntimeError("404") + + with ( + patch.object(downloader, "download_raw_file", side_effect=fake_download), + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object(gdv, "_ref_exists_via_ls_remote", return_value=(False, None)), + ): + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is True + + +class TestLegacyCollectionFallbackDispatch: + """`_is_legacy_collection_fallback` decides routing for SUBDIRECTORY paths.""" + + def test_apm_yml_present_means_not_a_legacy_collection(self) -> None: + """When `/apm.yml` exists, the dispatcher must NOT redirect to + the collection downloader (apm.yml takes priority).""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="collections/writing", ref="main") + + def fake_download(_dr, path, _ref): + if path == "collections/writing/apm.yml": + return b"name: writing\nversion: 1.0.0\n" + raise RuntimeError("404") + + with patch.object(downloader, "download_raw_file", side_effect=fake_download): + assert downloader._is_legacy_collection_fallback(dep_ref) is False + + def test_only_collection_yml_means_legacy_collection(self) -> None: + """No `/apm.yml` but `.collection.yml` exists -> legacy collection.""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="collections/writing", ref="main") + + def fake_download(_dr, path, _ref): + if path == "collections/writing.collection.yml": + return b"description: legacy\nitems: []\n" + raise RuntimeError("404") + + with patch.object(downloader, "download_raw_file", side_effect=fake_download): + assert downloader._is_legacy_collection_fallback(dep_ref) is True + + def test_neither_present_under_collections_path_no_fallback(self) -> None: + """A `collections/` path with no apm.yml and no .collection.yml + means no legacy collection (caller falls through to the regular + subdirectory clone path).""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="collections/whatever", ref="main") + + with patch.object(downloader, "download_raw_file", side_effect=RuntimeError("404")): + assert downloader._is_legacy_collection_fallback(dep_ref) is False + + def test_only_runs_for_subdirectory_refs(self) -> None: + """Explicit COLLECTION refs route directly; the helper short-circuits.""" + downloader = GitHubPackageDownloader() + dep_ref = DependencyReference( + repo_url="owner/repo", + virtual_path="collections/writing.collection.yml", + is_virtual=True, + reference="main", + ) + # No download_raw_file call should happen at all. + with patch.object(downloader, "download_raw_file") as raw: + assert downloader._is_legacy_collection_fallback(dep_ref) is False + raw.assert_not_called() + + def test_path_outside_collections_short_circuits_without_http(self) -> None: + """Perf hint: SUBDIRECTORY refs without a `collections/` segment + return immediately without any HTTP probe. + + This optimisation prevents skill installs (e.g. ``skills/my-skill``) + from paying 2-3 wasted HTTP probes per install for a legacy + collection fallback they could never trigger. + """ + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/my-skill", ref="main") + with patch.object(downloader, "download_raw_file") as raw: + assert downloader._is_legacy_collection_fallback(dep_ref) is False + raw.assert_not_called() + + def test_collections_prefix_path_runs_probes(self) -> None: + """A path STARTING with `collections/` triggers the probes.""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="collections/foo", ref="main") + with patch.object(downloader, "download_raw_file", side_effect=RuntimeError("404")) as raw: + assert downloader._is_legacy_collection_fallback(dep_ref) is False + # apm.yml probe + 2 collection probes = 3 calls total. + assert raw.call_count == 3 diff --git a/tests/unit/test_ado_path_structure.py b/tests/unit/test_ado_path_structure.py index db4e51d0a..2a3bd4298 100644 --- a/tests/unit/test_ado_path_structure.py +++ b/tests/unit/test_ado_path_structure.py @@ -452,7 +452,9 @@ class TestADOVirtualPackagePaths: def test_github_virtual_package_uses_2_level_path(self): """Verify GitHub virtual packages install to 2-level path.""" - dep = DependencyReference.parse("owner/test-repo/collections/project-planning") + dep = DependencyReference.parse( + "owner/test-repo/collections/project-planning.collection.yml" + ) assert dep.is_virtual is True assert dep.is_virtual_collection() is True @@ -474,7 +476,7 @@ def test_github_virtual_package_uses_2_level_path(self): def test_ado_virtual_collection_uses_3_level_path(self): """Verify ADO virtual collections install to 3-level path.""" dep = DependencyReference.parse( - "dev.azure.com/myorg/myproject/myrepo/collections/my-collection" + "dev.azure.com/myorg/myproject/myrepo/collections/my-collection.collection.yml" ) assert dep.is_azure_devops() is True @@ -524,14 +526,15 @@ def test_ado_virtual_file_uses_3_level_path(self): def test_ado_collection_with_git_segment(self): """Verify ADO collections with _git segment work correctly.""" dep = DependencyReference.parse( - "dev.azure.com/myorg/myproject/_git/copilot-instructions/collections/csharp-ddd" + "dev.azure.com/myorg/myproject/_git/copilot-instructions/collections/" + "csharp-ddd.collection.yml" ) assert dep.is_azure_devops() is True assert dep.is_virtual is True assert dep.is_virtual_collection() is True assert dep.repo_url == "myorg/myproject/copilot-instructions" - assert dep.virtual_path == "collections/csharp-ddd" + assert dep.virtual_path == "collections/csharp-ddd.collection.yml" # Verify correct install path repo_parts = dep.repo_url.split("/") diff --git a/tests/unit/test_artifactory_support.py b/tests/unit/test_artifactory_support.py index f6b9043d5..a653dc1a7 100644 --- a/tests/unit/test_artifactory_support.py +++ b/tests/unit/test_artifactory_support.py @@ -903,7 +903,8 @@ def test_explicit_artifactory_fqdn_virtual_collection_passes(self): with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): dl = GitHubPackageDownloader() dep = DependencyReference.parse( - "art.example.com/artifactory/github/owner/repo/collections/my-collection" + "art.example.com/artifactory/github/owner/repo/collections/" + "my-collection.collection.yml" ) assert dep.is_artifactory() assert dep.is_virtual_collection() diff --git a/tests/unit/test_generic_git_urls.py b/tests/unit/test_generic_git_urls.py index 2c7d448bd..9ac79bcce 100644 --- a/tests/unit/test_generic_git_urls.py +++ b/tests/unit/test_generic_git_urls.py @@ -455,10 +455,12 @@ def test_gitlab_virtual_file(self): assert dep.is_virtual_file() is True def test_bitbucket_virtual_collection(self): - dep = DependencyReference.parse("bitbucket.org/team/rules/collections/security") + dep = DependencyReference.parse( + "bitbucket.org/team/rules/collections/security.collection.yml" + ) assert dep.host == "bitbucket.org" assert dep.repo_url == "team/rules" - assert dep.virtual_path == "collections/security" + assert dep.virtual_path == "collections/security.collection.yml" assert dep.is_virtual is True assert dep.is_virtual_collection() is True diff --git a/tests/unit/test_meta_package.py b/tests/unit/test_meta_package.py new file mode 100644 index 000000000..21182a35e --- /dev/null +++ b/tests/unit/test_meta_package.py @@ -0,0 +1,162 @@ +"""Unit tests for META_PACKAGE detection and validation (#1094). + +A meta-package is a curated dependency aggregator: ``apm.yml`` declares +``dependencies.apm`` and/or ``dependencies.mcp`` and contributes no own +primitives (no ``.apm/``, no ``SKILL.md``, no nested skills). This shape +existed in practice as a workaround that required an empty ``.apm/.gitkeep``; +issue #1094 promoted it to a first-class type so users no longer need to +commit a placeholder directory just to satisfy the structural check. +""" + +from pathlib import Path + +from src.apm_cli.models.apm_package import ( + PackageType, + validate_apm_package, +) +from src.apm_cli.models.validation import detect_package_type + + +class TestMetaPackageDetection: + """Detection cascade: META_PACKAGE classification.""" + + def _write_apm_yml(self, tmp_path: Path, body: str) -> None: + (tmp_path / "apm.yml").write_text(body) + + def test_apm_yml_with_apm_deps_detected_as_meta(self, tmp_path): + """apm.yml + non-empty dependencies.apm + no .apm/ -> META_PACKAGE.""" + self._write_apm_yml( + tmp_path, + "name: writing\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - owner/repo/skills/foo\n" + " mcp: []\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.META_PACKAGE + + def test_apm_yml_with_dev_deps_only_detected_as_meta(self, tmp_path): + """A dev-only dep aggregator is still a meta-package.""" + self._write_apm_yml( + tmp_path, + "name: dev-bundle\nversion: 1.0.0\ndevDependencies:\n apm:\n - some/dev-tool\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.META_PACKAGE + + def test_apm_yml_with_mcp_deps_only_detected_as_meta(self, tmp_path): + """apm.yml with only mcp deps and no .apm/ still meta.""" + self._write_apm_yml( + tmp_path, + "name: mcp-bundle\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm: []\n" + " mcp:\n" + " - some/mcp-server\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.META_PACKAGE + + def test_apm_yml_no_deps_no_apm_dir_still_invalid(self, tmp_path): + """apm.yml with no deps and no .apm/ stays INVALID (the warning case).""" + self._write_apm_yml(tmp_path, "name: empty\nversion: 1.0.0\n") + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.INVALID + + def test_apm_yml_empty_deps_dict_still_invalid(self, tmp_path): + """apm.yml with `dependencies: {apm: [], mcp: []}` -> INVALID.""" + self._write_apm_yml( + tmp_path, + "name: empty-deps\nversion: 1.0.0\ndependencies:\n apm: []\n mcp: []\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.INVALID + + def test_apm_yml_with_apm_dir_still_apm_package(self, tmp_path): + """apm.yml + .apm/ + deps -> APM_PACKAGE (.apm/ wins over deps signal).""" + self._write_apm_yml( + tmp_path, + "name: real\nversion: 1.0.0\ndependencies:\n apm:\n - owner/repo/skills/foo\n", + ) + (tmp_path / ".apm").mkdir() + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.APM_PACKAGE + + def test_apm_yml_meta_with_skill_bundle_still_skill_bundle(self, tmp_path): + """Nested skills//SKILL.md takes priority over META_PACKAGE.""" + self._write_apm_yml( + tmp_path, + "name: bundle\nversion: 1.0.0\ndependencies:\n apm:\n - owner/repo/skills/foo\n", + ) + skills_dir = tmp_path / "skills" / "my-skill" + skills_dir.mkdir(parents=True) + (skills_dir / "SKILL.md").write_text( + "---\nname: my-skill\ndescription: A test skill\n---\n# Skill\n" + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.SKILL_BUNDLE + + def test_apm_yml_with_skill_md_root_still_hybrid(self, tmp_path): + """Root SKILL.md + apm.yml + deps -> HYBRID (root SKILL.md wins).""" + self._write_apm_yml( + tmp_path, + "name: hybrid\nversion: 1.0.0\ndependencies:\n apm:\n - owner/repo/skills/foo\n", + ) + (tmp_path / "SKILL.md").write_text( + "---\nname: root\ndescription: root skill\n---\n# Root\n" + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.HYBRID + + def test_malformed_apm_yml_treated_as_invalid(self, tmp_path): + """Tolerant of unparseable apm.yml: INVALID, not META_PACKAGE.""" + self._write_apm_yml(tmp_path, "name: bad\nversion: 1.0.0\ndependencies: not-a-dict\n") + pkg_type, _ = detect_package_type(tmp_path) + # No .apm/, no nested skills, deps unparseable -> falls through to INVALID + # so the user sees the standard "missing .apm/" diagnostic. + assert pkg_type == PackageType.INVALID + + +class TestMetaPackageValidation: + """Full validate_apm_package: META_PACKAGE passes cleanly.""" + + def test_meta_package_passes_validation(self, tmp_path): + """META_PACKAGE validation succeeds without `.apm/`.""" + (tmp_path / "apm.yml").write_text( + "name: writing\n" + "version: 1.0.0\n" + "description: Curated writing-skills bundle\n" + "dependencies:\n apm:\n - owner/repo/skills/foo\n", + ) + result = validate_apm_package(tmp_path) + assert result.is_valid is True + assert result.package_type == PackageType.META_PACKAGE + assert result.package is not None + assert result.package.name == "writing" + assert result.package.version == "1.0.0" + # Validation does not require `.apm/` for META_PACKAGE. + assert not (tmp_path / ".apm").exists() + + def test_meta_package_no_missing_apm_dir_error(self, tmp_path): + """The legacy `missing .apm/` error must NOT fire for META_PACKAGE.""" + (tmp_path / "apm.yml").write_text( + "name: writing\nversion: 1.0.0\ndependencies:\n apm:\n - owner/repo/skills/foo\n", + ) + result = validate_apm_package(tmp_path) + for err in result.errors: + assert "missing the required .apm/ directory" not in err + assert "Missing required directory: .apm/" not in err + + def test_invalid_apm_yml_still_errors_for_meta(self, tmp_path): + """A META_PACKAGE with malformed apm.yml structure surfaces the parse error.""" + # Pass detect_package_type by declaring deps, but break the from_apm_yml + # parser by omitting the required `version` field. + (tmp_path / "apm.yml").write_text( + "name: writing\ndependencies:\n apm:\n - owner/repo/skills/foo\n", + ) + result = validate_apm_package(tmp_path) + assert result.is_valid is False + assert any("Invalid apm.yml" in err for err in result.errors) diff --git a/tests/unit/test_package_identity.py b/tests/unit/test_package_identity.py index c7bf940e7..5f1f21f29 100644 --- a/tests/unit/test_package_identity.py +++ b/tests/unit/test_package_identity.py @@ -137,22 +137,42 @@ def test_virtual_file_package(self): assert dep.get_install_path(apm_modules) == expected def test_virtual_collection_package(self): - """Virtual collection: apm_modules/owner/.""" - dep = DependencyReference.parse("owner/test-repo/collections/azure-cloud-development") + """Explicit `.collection.yml` URL: apm_modules/owner/. + + Per #1094, only URLs ending in `.collection.yml`/`.yaml` are + eagerly classified as COLLECTION (and use the flattened install + path). Implicit `collections/` URLs are SUBDIRECTORY and + use the natural path layout (covered separately below). + """ + dep = DependencyReference.parse( + "owner/test-repo/collections/azure-cloud-development.collection.yml" + ) apm_modules = Path("/project/apm_modules") - # Virtual package name: test-repo-azure-cloud-development + # Virtual package name: test-repo-azure-cloud-development (flattened) expected = apm_modules / "owner" / "test-repo-azure-cloud-development" assert dep.get_install_path(apm_modules) == expected def test_virtual_collection_with_reference(self): """Reference does not affect virtual package install path.""" - dep = DependencyReference.parse("owner/test-repo/collections/testing#main") + dep = DependencyReference.parse("owner/test-repo/collections/testing.collection.yml#main") apm_modules = Path("/project/apm_modules") expected = apm_modules / "owner" / "test-repo-testing" assert dep.get_install_path(apm_modules) == expected + def test_collections_path_subdirectory_uses_natural_layout(self): + """`/collections/` (no extension) is SUBDIRECTORY (#1094). + + SUBDIRECTORY install paths mirror the repo path so an actual + `collections//apm.yml` package lives at the natural location + under apm_modules/, instead of the flattened collection layout. + """ + dep = DependencyReference.parse("owner/test-repo/collections/azure-cloud-development") + apm_modules = Path("/project/apm_modules") + expected = apm_modules / "owner" / "test-repo" / "collections" / "azure-cloud-development" + assert dep.get_install_path(apm_modules) == expected + def test_ado_regular_package(self): """ADO regular package: apm_modules/org/project/repo.""" dep = DependencyReference.parse("dev.azure.com/myorg/myproject/myrepo") @@ -172,13 +192,13 @@ def test_ado_virtual_package(self): assert dep.get_install_path(apm_modules) == expected def test_ado_virtual_collection(self): - """ADO virtual collection: apm_modules/org/project/.""" + """ADO explicit `.collection.yml` URL: apm_modules/org/project/.""" dep = DependencyReference.parse( - "dev.azure.com/myorg/myproject/myrepo/collections/my-collection" + "dev.azure.com/myorg/myproject/myrepo/collections/my-collection.collection.yml" ) apm_modules = Path("/project/apm_modules") - # Virtual package name: myrepo-my-collection + # Virtual package name: myrepo-my-collection (flattened) expected = apm_modules / "myorg" / "myproject" / "myrepo-my-collection" assert dep.get_install_path(apm_modules) == expected @@ -194,10 +214,17 @@ class TestInstallPathConsistency: """Test that get_install_path is consistent with virtual package naming.""" def test_consistency_with_get_virtual_package_name(self): - """Install path uses same package name as get_virtual_package_name.""" + """Install path's last segment equals get_virtual_package_name() for + flattened-layout virtual refs (FILE / explicit COLLECTION). + + SUBDIRECTORY refs use a natural-layout install path that mirrors + the repo structure, so the last-segment invariant does not hold + for them; that case is covered separately by + ``test_collections_path_subdirectory_uses_natural_layout``. + """ test_cases = [ "owner/test-repo/prompts/code-review.prompt.md", - "owner/test-repo/collections/azure-cloud-development", + "owner/test-repo/collections/azure-cloud-development.collection.yml", "owner/repo/agents/security.agent.md", "user/pkg/instructions/coding.instructions.md", ] @@ -242,8 +269,14 @@ class TestUninstallScenarios: """Test scenarios that were broken before the fix.""" def test_uninstall_virtual_collection_finds_correct_path(self): - """Uninstalling virtual collection should find owner/virtual-pkg-name, not owner/repo/collections/name.""" - dep_str = "owner/test-repo/collections/azure-cloud-development" + """Explicit `.collection.yml` URL still installs to the flattened + (collection) layout so uninstall logic targeting that layout works. + + Implicit `collections/` URLs are SUBDIRECTORY (#1094) and use + the natural-layout path -- a separate flow with its own uninstall + logic, exercised by the SUBDIRECTORY tests. + """ + dep_str = "owner/test-repo/collections/azure-cloud-development.collection.yml" dep = DependencyReference.parse(dep_str) apm_modules = Path("apm_modules") diff --git a/tests/unit/test_script_runner.py b/tests/unit/test_script_runner.py index f9a359ea4..a8ccf326d 100644 --- a/tests/unit/test_script_runner.py +++ b/tests/unit/test_script_runner.py @@ -622,7 +622,10 @@ def test_auto_install_virtual_package_collection_success( mock_downloader.download_virtual_collection_package.return_value = mock_package_info # Test auto-install - ref = "owner/test-repo/collections/project-planning" + # Use the explicit `.collection.yml` URL form so the dep is classified + # as COLLECTION up-front (per #1094, the implicit `collections/` + # form is now SUBDIRECTORY and resolved at fetch time). + ref = "owner/test-repo/collections/project-planning.collection.yml" result = self.script_runner._auto_install_virtual_package(ref) assert result is True From 30deb8215033f99e071499cb69836c65ec14d12a Mon Sep 17 00:00:00 2001 From: xuyuanhao Date: Fri, 1 May 2026 23:50:29 +0800 Subject: [PATCH 2/5] fix(deps): tighten META_PACKAGE detection and sync docs Address review feedback: * Require `dependencies.{apm,mcp}` (and `devDependencies.{apm,mcp}`) to be a non-empty list before classifying a package as META_PACKAGE. A truthy non-list value (e.g. `apm: "foo"` or `apm: {}`) is malformed per the schema; treat it as INVALID so the legacy diagnostic still fires instead of silently reclassifying. Adds two regression tests. * Update `validate_apm_package` docstring to list META_PACKAGE among the supported package types. * Update user-facing docs (manifest-schema.md, dependencies.md) to describe the extension-only classification rule and remove the old "Collection (dir)" entry; mention the meta_package value in the apm.lock.yaml `package_type` enumeration. --- .../content/docs/reference/manifest-schema.md | 9 +++++---- .../.apm/skills/apm-usage/dependencies.md | 7 ++++--- src/apm_cli/models/validation.py | 14 ++++++++++++-- tests/unit/test_meta_package.py | 18 ++++++++++++++++++ 4 files changed, 39 insertions(+), 9 deletions(-) diff --git a/docs/src/content/docs/reference/manifest-schema.md b/docs/src/content/docs/reference/manifest-schema.md index ed8eb38b7..eee2f0ea9 100644 --- a/docs/src/content/docs/reference/manifest-schema.md +++ b/docs/src/content/docs/reference/manifest-schema.md @@ -329,9 +329,10 @@ A dependency MAY target a subdirectory, file, or collection within a repository | Kind | Detection rule | Example | |---|---|---| | **File** | `virtual_path` ends in `.prompt.md`, `.instructions.md`, `.agent.md`, or `.chatmode.md` | `owner/repo/prompts/review.prompt.md` | -| **Collection (dir)** | `virtual_path` contains `/collections/` (no collection extension) | `owner/repo/collections/security` | -| **Collection (manifest)** | `virtual_path` contains `/collections/` and ends with `.collection.yml` or `.collection.yaml` | `owner/repo/collections/security.collection.yml` | -| **Subdirectory** | `virtual_path` does not match any file, collection, or extension rule above | `owner/repo/skills/security` | +| **Collection (manifest)** | `virtual_path` ends with `.collection.yml` or `.collection.yaml` | `owner/repo/collections/security.collection.yml` | +| **Subdirectory** | `virtual_path` does not match any file or collection extension above | `owner/repo/skills/security` | + +Classification is by extension only -- never by path segment. A path like `owner/repo/collections/security` (no extension) is **Subdirectory**, not Collection: the actual on-disk shape (APM package, skill bundle, plugin, or legacy collection) is resolved at fetch time by probing for `apm.yml` first and falling back to a sibling `security.collection.yml` for the legacy form. #### 4.1.4. Canonical Normalisation @@ -527,7 +528,7 @@ dependencies: # YAML list (not a map) is_virtual: # True for virtual (file/subdirectory) packages depth: # 1 = direct, 2+ = transitive resolved_by: # Parent dependency (transitive only) - package_type: # Package type (e.g. "apm_package", "marketplace_plugin") + package_type: # Package type (e.g. "apm_package", "marketplace_plugin", "meta_package") content_hash: # SHA-256 of package file tree (e.g. "sha256:a1b2c3...") is_dev: # True for devDependencies deployed_files: > # Workspace-relative paths of installed files diff --git a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md index 5a0eea5c7..7c3781662 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md +++ b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md @@ -116,9 +116,10 @@ Virtual packages reference a subset of a repository. | Type | Detection rule | Example | |------|---------------|---------| | File | Ends in `.prompt.md`, `.instructions.md`, `.agent.md`, `.chatmode.md` | `owner/repo/prompts/review.prompt.md` | -| Collection (dir) | Contains `/collections/` (no extension) | `owner/repo/collections/security` | -| Collection (manifest) | Contains `/collections/` + `.collection.yml` | `owner/repo/collections/security.collection.yml` | -| Subdirectory | Does not match file or collection rules | `owner/repo/skills/security` | +| Collection (manifest) | Ends with `.collection.yml` or `.collection.yaml` | `owner/repo/collections/security.collection.yml` | +| Subdirectory | Does not match a file or collection extension above | `owner/repo/skills/security` | + +Classification is by extension only. A path like `owner/repo/collections/security` (no extension) is a Subdirectory; the actual shape -- APM package, meta-package (`apm.yml` declaring deps with no `.apm/`), skill bundle, plugin, or legacy collection -- is resolved at fetch time. APM probes for `apm.yml` first and falls back to a sibling `security.collection.yml` for the legacy form. ## Canonical storage rules diff --git a/src/apm_cli/models/validation.py b/src/apm_cli/models/validation.py index 2c0b9162e..cc9675a3f 100644 --- a/src/apm_cli/models/validation.py +++ b/src/apm_cli/models/validation.py @@ -313,7 +313,15 @@ def _apm_yml_declares_dependencies(apm_yml_path: Path) -> bool: def _has_listed_deps(block: object) -> bool: if not isinstance(block, dict): return False - return bool(block.get("apm") or block.get("mcp")) + # Schema requires `apm` and `mcp` to be lists. Strings, dicts, and + # other truthy non-list values are malformed; treat them as "no + # declared dependencies" so the caller falls through to the legacy + # INVALID diagnostic instead of silently reclassifying as META. + for key in ("apm", "mcp"): + value = block.get(key) + if isinstance(value, list) and value: + return True + return False return _has_listed_deps(data.get("dependencies")) or _has_listed_deps( data.get("devDependencies") @@ -323,13 +331,15 @@ def _has_listed_deps(block: object) -> bool: def validate_apm_package(package_path: Path) -> ValidationResult: """Validate that a directory contains a valid APM package or Claude Skill. - Supports six package types: + Supports seven package types: - APM_PACKAGE: Has apm.yml and .apm/ directory - CLAUDE_SKILL: Has SKILL.md but no apm.yml (auto-generates apm.yml) - HOOK_PACKAGE: Has hooks/*.json but no apm.yml or SKILL.md - MARKETPLACE_PLUGIN: Has plugin.json or .claude-plugin/ (synthesizes apm.yml) - HYBRID: Has both apm.yml and root SKILL.md - SKILL_BUNDLE: Has skills//SKILL.md, apm.yml optional + - META_PACKAGE: apm.yml declares deps but no .apm/ -- a curated dep + aggregator with no own primitives (#1094) Args: package_path: Path to the directory to validate diff --git a/tests/unit/test_meta_package.py b/tests/unit/test_meta_package.py index 21182a35e..67ef9f1d2 100644 --- a/tests/unit/test_meta_package.py +++ b/tests/unit/test_meta_package.py @@ -119,6 +119,24 @@ def test_malformed_apm_yml_treated_as_invalid(self, tmp_path): # so the user sees the standard "missing .apm/" diagnostic. assert pkg_type == PackageType.INVALID + def test_apm_string_value_is_invalid_not_meta(self, tmp_path): + """Schema requires apm to be a list; a string value is malformed -> INVALID.""" + self._write_apm_yml( + tmp_path, + "name: malformed\nversion: 1.0.0\ndependencies:\n apm: foo\n mcp: []\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.INVALID + + def test_apm_dict_value_is_invalid_not_meta(self, tmp_path): + """Schema requires apm to be a list; a dict value is malformed -> INVALID.""" + self._write_apm_yml( + tmp_path, + "name: malformed\nversion: 1.0.0\ndependencies:\n apm:\n key: value\n mcp: []\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.INVALID + class TestMetaPackageValidation: """Full validate_apm_package: META_PACKAGE passes cleanly.""" From f1b8109a7b8e923c754daa540374ab2aff21f407 Mon Sep 17 00:00:00 2001 From: Copilot Date: Sat, 2 May 2026 10:54:24 +0200 Subject: [PATCH 3/5] fix(deps,validation): drop collection-virtual-package, accept dep-only apm.yml (#1094) Reworks PR #1097 per the apm-review-panel synthesis. The conceptual fix for issue #1094 is a one-line guard relaxation in `_validate_apm_package_with_yml`: a curated dependency aggregator (apm.yml with declared deps and no .apm/) is now a valid APM_PACKAGE, removing the need for users to commit an empty `.apm/.gitkeep`. The original PR also introduced a parallel `.collection.yml` / `VirtualPackageType.COLLECTION` codepath that overlapped with the existing SUBDIRECTORY virtual-package handling and added a latent production bug (`download_virtual_collection_package` was referenced from `script_runner.py` but never existed on the downloader). The panel agreed: delete the collection-virtual-package machinery wholesale, since the relaxed validation makes it redundant. Lockfiles are unaffected -- the COLLECTION enum value was never persisted. Changes: - Validation cascade collapses to: any apm.yml -> APM classification. With .apm/ OR declared dependencies, APM_PACKAGE; otherwise INVALID (preserving the standard "missing .apm/" diagnostic). The dep-list detection requires at least one parseable str/dict entry to guard against malformed manifests like `apm: [123]`. - DependencyReference.parse() now raises ValueError at parse time for any URL ending in `.collection.yml` / `.collection.yaml`, with a migration message pointing at the dependencies guide. - Removed: VirtualPackageType.COLLECTION, is_virtual_collection(), download_collection_package, normalize_collection_path, _is_legacy_collection_fallback, the collection probe in github_downloader_validation, collection_parser.py, the META_PACKAGE install-source label. - Docs: manifest-schema.md, dependencies.md, CHANGELOG.md updated with migration callouts. - Tests: 178 new lines covering dep-only detection (incl. malformed shapes), 6 regression-trap tests for the migration error path, 6 test files adapted to the SUBDIRECTORY-only world; integration test_collection_install.py and unit test_meta_package.py removed. Closes #1094. Co-authored-by: xuyuanhao Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 + .../content/docs/reference/manifest-schema.md | 13 +- .../.apm/skills/apm-usage/dependencies.md | 7 +- src/apm_cli/core/script_runner.py | 11 +- src/apm_cli/deps/collection_parser.py | 125 -------- src/apm_cli/deps/github_downloader.py | 277 +----------------- .../deps/github_downloader_validation.py | 18 +- src/apm_cli/install/sources.py | 1 - src/apm_cli/integration/skill_integrator.py | 2 +- src/apm_cli/models/dependency/reference.py | 70 +++-- src/apm_cli/models/dependency/types.py | 1 - src/apm_cli/models/validation.py | 114 +++---- tests/integration/test_collection_install.py | 245 ---------------- tests/test_apm_package_models.py | 44 +-- tests/test_github_downloader.py | 86 ------ .../deps/test_github_downloader_validation.py | 158 +--------- tests/unit/test_ado_path_structure.py | 27 +- tests/unit/test_artifactory_support.py | 20 +- tests/unit/test_collection_migration_error.py | 51 ++++ ...ta_package.py => test_dep_only_package.py} | 107 +++++-- tests/unit/test_generic_git_urls.py | 17 +- tests/unit/test_package_identity.py | 65 +--- tests/unit/test_script_runner.py | 30 -- 23 files changed, 293 insertions(+), 1200 deletions(-) delete mode 100644 src/apm_cli/deps/collection_parser.py delete mode 100644 tests/integration/test_collection_install.py create mode 100644 tests/unit/test_collection_migration_error.py rename tests/unit/{test_meta_package.py => test_dep_only_package.py} (59%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86bbc0a66..face3126f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Removed + +- **BREAKING: dropped support for `.collection.yml` / `.collection.yaml` virtual packages.** Dependencies whose paths end in `.collection.yml` or `.collection.yaml` now raise a `ValueError` at parse time with a migration message. Convert any such entry to a regular `apm.yml` with a `dependencies:` section under the same subdirectory, then reference the directory itself as a subdirectory virtual package (no extension). The internal `VirtualPackageType.COLLECTION` enum value, the `download_collection_package` codepath, the `is_virtual_collection()` reference helper, and the `META_PACKAGE` package-type label have been removed -- none were persisted to the lockfile, so existing locks are unaffected. (#1097, closes #1094) Thanks @edenfunf for the original PR. + ### Changed - **Renamed `NOTICE.md` -> `NOTICE`** to follow the Apache / CNCF convention used by upstream third-party-attribution files (e.g. `kubernetes-sigs/kro`, `kubernetes-sigs/headlamp`). The generator (`scripts/generate-notice.py`), `make notice` target, and `NOTICE Drift Check` workflow now operate on the extension-less path. (#1073) diff --git a/docs/src/content/docs/reference/manifest-schema.md b/docs/src/content/docs/reference/manifest-schema.md index eee2f0ea9..a4b5e1dda 100644 --- a/docs/src/content/docs/reference/manifest-schema.md +++ b/docs/src/content/docs/reference/manifest-schema.md @@ -257,7 +257,7 @@ local_path_form = ("./" / "../" / "/" / "~/" / ".\\" / "..\\" / "~\\") path | `host` | OPTIONAL | FQDN (e.g. `gitlab.com`) | Git host. Defaults to `github.com`. | | `port` | OPTIONAL | `1`–`65535` | Non-default port on `ssh://`, `https://`, `http://` clone URLs. Not expressible in SCP shorthand. | | `owner/repo` | REQUIRED | 2+ path segments of `[a-zA-Z0-9._-]+` | Repository path. GitHub uses exactly 2 segments (`owner/repo`). Non-GitHub hosts MAY use nested groups (e.g. `gitlab.com/group/sub/repo`). | -| `virtual_path` | OPTIONAL | Path segments after repo | Subdirectory, file, or collection within the repo. See §4.1.3. | +| `virtual_path` | OPTIONAL | Path segments after repo | Subdirectory or file within the repo. See §4.1.3. | | `ref` | OPTIONAL | Branch, tag, or commit SHA | Git reference. Commit SHAs matched by `^[a-f0-9]{7,40}$`. Semver tags matched by `^v?\d+\.\d+\.\d+`. | **Examples:** @@ -303,7 +303,7 @@ REQUIRED when the shorthand is ambiguous (e.g. nested-group repos with virtual p | Field | Type | Required | Pattern / Constraint | Description | |---|---|---|---|---| | `git` | `string` | REQUIRED (remote) | HTTPS URL, SSH URL, or FQDN shorthand | Clone URL of the repository. Required for remote dependencies. | -| `path` | `string` | OPTIONAL / REQUIRED (local) | Relative path within the repo, or local filesystem path | When `git` is present: subdirectory, file, or collection (virtual package). When `git` is absent: local filesystem path (must start with `./`, `../`, `/`, or `~/`). | +| `path` | `string` | OPTIONAL / REQUIRED (local) | Relative path within the repo, or local filesystem path | When `git` is present: subdirectory or file (virtual package). When `git` is absent: local filesystem path (must start with `./`, `../`, `/`, or `~/`). | | `ref` | `string` | OPTIONAL | Branch, tag, or commit SHA | Git reference to checkout. | | `alias` | `string` | OPTIONAL | `^[a-zA-Z0-9._-]+$` | Local alias. | @@ -324,15 +324,16 @@ Local path dependency (development only): #### 4.1.3. Virtual Packages -A dependency MAY target a subdirectory, file, or collection within a repository rather than the whole repo. Conforming resolvers MUST classify virtual packages using the following rules, evaluated in order: +A dependency MAY target a subdirectory or a file within a repository rather than the whole repo. Conforming resolvers MUST classify virtual packages using the following rules, evaluated in order: | Kind | Detection rule | Example | |---|---|---| | **File** | `virtual_path` ends in `.prompt.md`, `.instructions.md`, `.agent.md`, or `.chatmode.md` | `owner/repo/prompts/review.prompt.md` | -| **Collection (manifest)** | `virtual_path` ends with `.collection.yml` or `.collection.yaml` | `owner/repo/collections/security.collection.yml` | -| **Subdirectory** | `virtual_path` does not match any file or collection extension above | `owner/repo/skills/security` | +| **Subdirectory** | `virtual_path` does not match any file extension above | `owner/repo/skills/security` | -Classification is by extension only -- never by path segment. A path like `owner/repo/collections/security` (no extension) is **Subdirectory**, not Collection: the actual on-disk shape (APM package, skill bundle, plugin, or legacy collection) is resolved at fetch time by probing for `apm.yml` first and falling back to a sibling `security.collection.yml` for the legacy form. +Classification is by extension only -- never by path segment. A path like `owner/repo/collections/security` (no extension) is **Subdirectory**: the actual on-disk shape (APM package with `apm.yml`, skill bundle, or plugin) is resolved at fetch time by probing for `apm.yml` first. + +> **Removed (#1094):** the legacy `.collection.yml` / `.collection.yaml` virtual-package form is no longer supported. Convert any such reference to an `apm.yml` with a `dependencies:` section, then reference the resulting subdirectory as a regular subdirectory virtual package. #### 4.1.4. Canonical Normalisation diff --git a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md index 7c3781662..6db54055c 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md +++ b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md @@ -116,10 +116,11 @@ Virtual packages reference a subset of a repository. | Type | Detection rule | Example | |------|---------------|---------| | File | Ends in `.prompt.md`, `.instructions.md`, `.agent.md`, `.chatmode.md` | `owner/repo/prompts/review.prompt.md` | -| Collection (manifest) | Ends with `.collection.yml` or `.collection.yaml` | `owner/repo/collections/security.collection.yml` | -| Subdirectory | Does not match a file or collection extension above | `owner/repo/skills/security` | +| Subdirectory | Does not match a file extension above | `owner/repo/skills/security` | -Classification is by extension only. A path like `owner/repo/collections/security` (no extension) is a Subdirectory; the actual shape -- APM package, meta-package (`apm.yml` declaring deps with no `.apm/`), skill bundle, plugin, or legacy collection -- is resolved at fetch time. APM probes for `apm.yml` first and falls back to a sibling `security.collection.yml` for the legacy form. +Classification is by extension only. A path like `owner/repo/collections/security` (no extension) is a Subdirectory; the actual shape -- APM package (incl. dep-only `apm.yml` with no `.apm/`), skill bundle, or plugin -- is resolved at fetch time by probing for `apm.yml`. + +> **Removed (#1094):** the legacy `.collection.yml` / `.collection.yaml` virtual-package form is no longer supported. Convert any `.collection.yml` to an `apm.yml` with a `dependencies:` section, then reference the resulting subdirectory as a regular subdirectory virtual package. ## Canonical storage rules diff --git a/src/apm_cli/core/script_runner.py b/src/apm_cli/core/script_runner.py index 5f8d0bc46..ea86d2bcf 100644 --- a/src/apm_cli/core/script_runner.py +++ b/src/apm_cli/core/script_runner.py @@ -783,8 +783,8 @@ def _is_virtual_package_reference(self, name: str) -> bool: Virtual packages have format: - owner/repo/path/to/file.prompt.md (virtual file) - - owner/repo/collections/name (virtual collection) - owner/repo/skills/name (virtual subdirectory/skill) + - owner/repo/collections/name (virtual subdirectory) Args: name: Name to check @@ -807,10 +807,9 @@ def _is_virtual_package_reference(self, name: str) -> bool: def _auto_install_virtual_package(self, package_ref: str) -> bool: """Auto-install a virtual package. - Handles three types of virtual packages: + Handles two types of virtual packages: - Virtual files: owner/repo/prompts/file.prompt.md - - Virtual collections: owner/repo/collections/name - - Virtual subdirectories (skills): owner/repo/skills/name + - Virtual subdirectories (skills, collections): owner/repo/skills/name Args: package_ref: Virtual package reference @@ -845,9 +844,7 @@ def _auto_install_virtual_package(self, package_ref: str) -> bool: print(f" Downloading from {dep_ref.to_github_url()}") - if dep_ref.is_virtual_collection(): - package_info = downloader.download_virtual_collection_package(dep_ref, target_path) - elif dep_ref.is_virtual_subdirectory(): + if dep_ref.is_virtual_subdirectory(): package_info = downloader.download_subdirectory_package(dep_ref, target_path) else: package_info = downloader.download_virtual_file_package(dep_ref, target_path) diff --git a/src/apm_cli/deps/collection_parser.py b/src/apm_cli/deps/collection_parser.py deleted file mode 100644 index 509efd122..000000000 --- a/src/apm_cli/deps/collection_parser.py +++ /dev/null @@ -1,125 +0,0 @@ -"""Parser for APM collection manifest files (.collection.yml).""" - -from dataclasses import dataclass -from typing import Any, Dict, List, Optional # noqa: F401, UP035 - -import yaml - - -@dataclass -class CollectionItem: - """Represents a single item in a collection manifest.""" - - path: str # Relative path to the file (e.g., "prompts/code-review.prompt.md") - kind: str # Type of primitive (e.g., "prompt", "instruction", "chat-mode") - - @property - def subdirectory(self) -> str: - """Get the .apm subdirectory for this item based on its kind. - - Returns: - str: Subdirectory name (e.g., "prompts", "instructions", "chatmodes") - """ - kind_to_subdir = { - "prompt": "prompts", - "instruction": "instructions", - "chat-mode": "chatmodes", - "chatmode": "chatmodes", - "agent": "agents", - "context": "contexts", - } - return kind_to_subdir.get(self.kind.lower(), "prompts") # Default to prompts - - -@dataclass -class CollectionManifest: - """Represents a parsed collection manifest (.collection.yml).""" - - id: str - name: str - description: str - items: list[CollectionItem] - tags: list[str] | None = None - display: dict[str, Any] | None = None - - @property - def item_count(self) -> int: - """Get the number of items in this collection.""" - return len(self.items) - - def get_items_by_kind(self, kind: str) -> list[CollectionItem]: - """Get all items of a specific kind. - - Args: - kind: The kind to filter by (e.g., "prompt", "instruction") - - Returns: - List of items matching the kind - """ - return [item for item in self.items if item.kind.lower() == kind.lower()] - - -def parse_collection_yml(content: bytes) -> CollectionManifest: - """Parse a collection YAML manifest. - - Args: - content: Raw YAML content as bytes - - Returns: - CollectionManifest: Parsed and validated collection manifest - - Raises: - ValueError: If the YAML is invalid or missing required fields - yaml.YAMLError: If YAML parsing fails - """ - try: - # Parse YAML - data = yaml.safe_load(content) - - if not isinstance(data, dict): - raise ValueError("Collection YAML must be a dictionary") - - # Validate required fields - required_fields = ["id", "name", "description", "items"] - missing_fields = [field for field in required_fields if field not in data] - - if missing_fields: - raise ValueError( - f"Collection manifest missing required fields: {', '.join(missing_fields)}" - ) - - # Validate items array - items_data = data.get("items", []) - if not isinstance(items_data, list): - raise ValueError("Collection 'items' must be a list") - - if not items_data: - raise ValueError("Collection must contain at least one item") - - # Parse items - items = [] - for idx, item_data in enumerate(items_data): - if not isinstance(item_data, dict): - raise ValueError(f"Collection item {idx} must be a dictionary") - - # Validate item fields - if "path" not in item_data: - raise ValueError(f"Collection item {idx} missing required field 'path'") - - if "kind" not in item_data: - raise ValueError(f"Collection item {idx} missing required field 'kind'") - - items.append(CollectionItem(path=item_data["path"], kind=item_data["kind"])) - - # Create manifest - return CollectionManifest( - id=data["id"], - name=data["name"], - description=data["description"], - items=items, - tags=data.get("tags"), - display=data.get("display"), - ) - - except yaml.YAMLError as e: - raise ValueError(f"Invalid YAML format: {e}") # noqa: B904 diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 5deb8b56c..300b89f49 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -70,25 +70,6 @@ ) -def normalize_collection_path(virtual_path: str) -> str: - """Normalize a collection virtual path by stripping any existing extension. - - This allows users to specify collection dependencies with or without the extension: - - owner/repo/collections/name (without extension) - - owner/repo/collections/name.collection.yml (with extension) - - Args: - virtual_path: The virtual path from the dependency reference - - Returns: - str: The normalized path without .collection.yml/.collection.yaml suffix - """ - for ext in (".collection.yml", ".collection.yaml"): - if virtual_path.endswith(ext): - return virtual_path[: -len(ext)] - return virtual_path - - def _debug(message: str) -> None: """Print debug message if APM_DEBUG environment variable is set.""" if os.environ.get("APM_DEBUG"): @@ -1421,174 +1402,6 @@ def download_virtual_file_package( dependency_ref=dep_ref, # Store for canonical dependency string ) - def download_collection_package( - self, - dep_ref: DependencyReference, - target_path: Path, - progress_task_id=None, - progress_obj=None, - ) -> PackageInfo: - """Download a collection as a virtual APM package. - - Downloads the collection manifest, then fetches all referenced files and - organizes them into the appropriate .apm/ subdirectories. - - Args: - dep_ref: Dependency reference with virtual_path pointing to collection - target_path: Local path where virtual package should be created - progress_task_id: Rich Progress task ID for progress updates - progress_obj: Rich Progress object for progress updates - - Returns: - PackageInfo: Information about the created virtual package - - Raises: - ValueError: If the dependency is not a valid collection package - RuntimeError: If download fails - """ - if not dep_ref.is_virtual or not dep_ref.virtual_path: - raise ValueError("Dependency must be a virtual collection package") - - # Accept both COLLECTION (explicit `.collection.yml` URL) and - # SUBDIRECTORY (legacy `/collections/` heuristic resolved at - # fetch time per #1094). The dispatcher in `download_package` - # decides which downloader to invoke; here we only reject - # non-virtual or shape-incompatible references. - if not (dep_ref.is_virtual_collection() or dep_ref.is_virtual_subdirectory()): - raise ValueError(f"Path '{dep_ref.virtual_path}' is not a valid collection path") - - # Determine the ref to use - ref = dep_ref.reference or "main" - - # Update progress - starting - if progress_obj and progress_task_id is not None: - progress_obj.update(progress_task_id, completed=10, total=100) - - # Normalize virtual_path by stripping .collection.yml/.yaml suffix if already present - # This allows users to specify either: - # - owner/repo/collections/name (without extension) - # - owner/repo/collections/name.collection.yml (with extension) - virtual_path_base = normalize_collection_path(dep_ref.virtual_path) - - # Extract collection name from normalized path (e.g., "collections/project-planning" -> "project-planning") - collection_name = virtual_path_base.split("/")[-1] - - # Build collection manifest path - try .yml first, then .yaml as fallback - collection_manifest_path = f"{virtual_path_base}.collection.yml" - - # Download the collection manifest - try: - manifest_content = self.download_raw_file(dep_ref, collection_manifest_path, ref) - except RuntimeError as e: - # Try .yaml extension as fallback - if ".collection.yml" in str(e): - collection_manifest_path = f"{virtual_path_base}.collection.yaml" - try: - manifest_content = self.download_raw_file( - dep_ref, collection_manifest_path, ref - ) - except RuntimeError: - raise RuntimeError( # noqa: B904 - f"Collection manifest not found: {virtual_path_base}.collection.yml (also tried .yaml)" - ) - else: - raise RuntimeError(f"Failed to download collection manifest: {e}") # noqa: B904 - - # Parse the collection manifest - from .collection_parser import parse_collection_yml - - try: - manifest = parse_collection_yml(manifest_content) - except (ValueError, Exception) as e: - raise RuntimeError(f"Invalid collection manifest '{collection_name}': {e}") # noqa: B904 - - # Create target directory structure - target_path.mkdir(parents=True, exist_ok=True) - - # Download all items from the collection - downloaded_count = 0 - failed_items = [] - total_items = len(manifest.items) - - for idx, item in enumerate(manifest.items): - # Update progress for each item - if progress_obj and progress_task_id is not None: - progress_percent = 20 + int((idx / total_items) * 70) # 20% to 90% - progress_obj.update(progress_task_id, completed=progress_percent, total=100) - - try: - # Download the file - item_content = self.download_raw_file(dep_ref, item.path, ref) - - # Determine subdirectory based on item kind - subdir = item.subdirectory - - # Create the subdirectory - apm_subdir = target_path / ".apm" / subdir - apm_subdir.mkdir(parents=True, exist_ok=True) - - # Write the file - filename = item.path.split("/")[-1] - file_path = apm_subdir / filename - file_path.write_bytes(item_content) - - downloaded_count += 1 - - except RuntimeError as e: - # Log the failure but continue with other items - failed_items.append(f"{item.path} ({e})") - continue - - # Check if we downloaded at least some items - if downloaded_count == 0: - error_msg = f"Failed to download any items from collection '{collection_name}'" - if failed_items: - error_msg += f". Failures:\n - " + "\n - ".join(failed_items) # noqa: F541 - raise RuntimeError(error_msg) - - # Generate apm.yml with collection metadata - package_name = dep_ref.get_virtual_package_name() - - apm_yml_data = { - "name": package_name, - "version": "1.0.0", - "description": manifest.description, - "author": dep_ref.repo_url.split("/")[0], - } - if manifest.tags: - apm_yml_data["tags"] = list(manifest.tags) - apm_yml_content = yaml_to_str(apm_yml_data) - - apm_yml_path = target_path / "apm.yml" - apm_yml_path.write_text(apm_yml_content, encoding="utf-8") - - # Create APMPackage object - package = APMPackage( - name=package_name, - version="1.0.0", - description=manifest.description, - author=dep_ref.repo_url.split("/")[0], - source=dep_ref.to_github_url(), - package_path=target_path, - ) - - # Log warnings for failed items if any - if failed_items: - import warnings - - warnings.warn( # noqa: B028 - f"Collection '{collection_name}' installed with {downloaded_count}/{manifest.item_count} items. " - f"Failed items: {len(failed_items)}" - ) - - # Return PackageInfo - return PackageInfo( - package=package, - install_path=target_path, - installed_at=datetime.now().isoformat(), - dependency_ref=dep_ref, # Store for canonical dependency string - ) - def _try_sparse_checkout( self, dep_ref: DependencyReference, @@ -1656,55 +1469,6 @@ def _try_sparse_checkout( _debug(f"Sparse-checkout failed: {e}") return False - def _is_legacy_collection_fallback(self, dep_ref: DependencyReference) -> bool: - """Return True iff this SUBDIRECTORY ref is a legacy collection ref. - - A legacy collection reference is a path like ``collections/`` - with NO ``apm.yml`` at the path but WITH a sibling - ``.collection.yml`` (or ``.yaml``) at the parent. Predates the - extension-based URL form ``collections/.collection.yml``. - - Resolution order matches `validate_virtual_package_exists` and the - issue #1094 acceptance criterion: ``apm.yml`` always wins; the - collection manifest is the documented fallback. The ``apm.yml`` - probe is intentionally first so it short-circuits the (cheap) but - still-unnecessary collection probes for the common case. - - Performance: the probes only run for paths that match the legacy - ``/collections/`` convention -- a path-shape *hint*, not a - classification. The actual shape is still confirmed by network - probes; this only narrows the search space so SUBDIRECTORY refs - outside the ``/collections/`` convention (skill bundles, plugin - repos, etc.) skip 2-3 wasted HTTP calls per install. - """ - if not dep_ref.is_virtual_subdirectory(): - return False - vpath = dep_ref.virtual_path - # Perf hint: legacy `.collection.yml` files conventionally live - # under a `collections/` segment. Skip the probes for paths that - # don't match this shape -- the dispatcher will fall through to - # the regular subdirectory clone path with no extra HTTP cost. - if "/collections/" not in vpath and not vpath.startswith("collections/"): - return False - ref = dep_ref.reference or "main" - # If `/apm.yml` exists, this is a real APM/meta package, not a - # legacy collection reference. Short-circuit immediately so we never - # mis-route a fixed-but-similarly-shaped layout to the collection - # downloader. - try: - self.download_raw_file(dep_ref, f"{vpath}/apm.yml", ref) - return False - except RuntimeError: - pass - # Probe sibling collection manifests in priority order. - for ext in (".collection.yml", ".collection.yaml"): - try: - self.download_raw_file(dep_ref, f"{vpath}{ext}", ref) - return True - except RuntimeError: - continue - return False - def download_subdirectory_package( self, dep_ref: DependencyReference, @@ -2141,37 +1905,20 @@ def download_package( return self.download_virtual_file_package( dep_ref, target_path, progress_task_id, progress_obj ) - elif dep_ref.is_virtual_collection(): - return self.download_collection_package( - dep_ref, target_path, progress_task_id, progress_obj + # SUBDIRECTORY (the only other virtual type after #1094 dropped + # the `.collection.yml` form): includes Artifactory modes. + if dep_ref.is_artifactory(): + proxy_info = (dep_ref.host, dep_ref.artifactory_prefix, "https") + return self._download_subdirectory_from_artifactory( + dep_ref, target_path, proxy_info, progress_task_id, progress_obj ) - elif dep_ref.is_virtual_subdirectory(): - # Mode 1: explicit Artifactory FQDN from lockfile - if dep_ref.is_artifactory(): - proxy_info = (dep_ref.host, dep_ref.artifactory_prefix, "https") - return self._download_subdirectory_from_artifactory( - dep_ref, target_path, proxy_info, progress_task_id, progress_obj - ) - # Mode 2: transparent proxy via env var (art_proxy computed above) - if self._is_artifactory_only() and art_proxy: - return self._download_subdirectory_from_artifactory( - dep_ref, target_path, art_proxy, progress_task_id, progress_obj - ) - # Legacy collection fallback (#1094): a SUBDIRECTORY reference - # whose path has no `apm.yml` but whose sibling - # `.collection.yml` exists is a legacy collection - # reference (predates the extension-based URL form). Route to - # the collection downloader instead of cloning a directory - # that does not exist. - if self._is_legacy_collection_fallback(dep_ref): - return self.download_collection_package( - dep_ref, target_path, progress_task_id, progress_obj - ) - return self.download_subdirectory_package( - dep_ref, target_path, progress_task_id, progress_obj + if self._is_artifactory_only() and art_proxy: + return self._download_subdirectory_from_artifactory( + dep_ref, target_path, art_proxy, progress_task_id, progress_obj ) - else: - raise ValueError(f"Unknown virtual package type for {dep_ref.virtual_path}") + return self.download_subdirectory_package( + dep_ref, target_path, progress_task_id, progress_obj + ) # Artifactory download path (Mode 1: explicit FQDN, Mode 2: transparent proxy) use_artifactory = dep_ref.is_artifactory() diff --git a/src/apm_cli/deps/github_downloader_validation.py b/src/apm_cli/deps/github_downloader_validation.py index 51fd4e626..40ddefc47 100644 --- a/src/apm_cli/deps/github_downloader_validation.py +++ b/src/apm_cli/deps/github_downloader_validation.py @@ -155,23 +155,13 @@ def _probe(path: str) -> bool: _log(f' [i] Validating virtual package at ref "{ref}": {dep_ref.repo_url}/{vpath}') - if dep_ref.is_virtual_collection(): - # COLLECTION refs always have an explicit `.collection.yml` / - # `.collection.yaml` extension at the end of vpath (per the - # extension-only classification in `virtual_type`). Probe the file - # directly -- appending another `.collection.yml` would produce a - # non-existent double-extension path. - return _probe(vpath) - if dep_ref.is_virtual_file(): return _probe(vpath) if dep_ref.is_virtual_subdirectory(): - # Probe order is intentional: apm.yml takes priority over the legacy - # `.collection.yml` fallback so a `collections//apm.yml` - # is recognised as an APM package, not as a missing collection - # manifest (microsoft/apm#1094). Sibling .collection.yml files at the - # parent path remain a recognised legacy form. + # Probe order: apm.yml first (a `collections//apm.yml` is the + # supported way to express a curated dependency aggregator -- see + # microsoft/apm#1094), then the standard primitive markers. marker_paths = [ f"{vpath}/apm.yml", f"{vpath}/SKILL.md", @@ -179,8 +169,6 @@ def _probe(path: str) -> bool: f"{vpath}/.github/plugin/plugin.json", f"{vpath}/.claude-plugin/plugin.json", f"{vpath}/.cursor-plugin/plugin.json", - f"{vpath}.collection.yml", - f"{vpath}.collection.yaml", f"{vpath}/README.md", ] for marker_path in marker_paths: diff --git a/src/apm_cli/install/sources.py b/src/apm_cli/install/sources.py index e62dafc37..35bf4fe9d 100644 --- a/src/apm_cli/install/sources.py +++ b/src/apm_cli/install/sources.py @@ -58,7 +58,6 @@ def _format_package_type_label(pkg_type) -> str | None: PackageType.APM_PACKAGE: "APM Package (apm.yml)", PackageType.HOOK_PACKAGE: "Hook Package (hooks/*.json only)", PackageType.SKILL_BUNDLE: "Skill Bundle (skills//SKILL.md)", - PackageType.META_PACKAGE: "Meta Package (apm.yml dependency aggregator)", }.get(pkg_type) diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index c38fb6380..8624b32bb 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -1092,7 +1092,7 @@ def integrate_package_skill( target_paths=sub_deployed, ) - # Skip virtual FILE and COLLECTION packages - they're individual files, not full packages + # Skip virtual FILE packages - they're individual files, not full packages # Multiple virtual files from the same repo would collide on skill name # BUT: subdirectory packages (like Claude Skills) SHOULD generate skills if package_info.dependency_ref and package_info.dependency_ref.is_virtual: diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index 8ec8781e8..f6fe5f174 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -37,7 +37,7 @@ class DependencyReference: reference: str | None = None # e.g., "main", "v1.0.0", "abc123" alias: str | None = None # Optional alias for the dependency virtual_path: str | None = None # Path for virtual packages (e.g., "prompts/file.prompt.md") - is_virtual: bool = False # True if this is a virtual package (individual file or collection) + is_virtual: bool = False # True if this is a virtual package (individual file or subdirectory) # Azure DevOps specific fields (ADO uses org/project/repo structure) ado_organization: str | None = None # e.g., "dmeppiel-org" @@ -65,12 +65,11 @@ class DependencyReference: ".agent.md", ) - # Explicit collection-manifest extensions. A virtual_path that ends in one - # of these is unambiguously a `.collection.yml` reference. Paths that - # merely contain a `collections/` segment are NOT classified here -- they - # are SUBDIRECTORY references whose actual shape is resolved at fetch - # time, with `.collection.yml` as a documented fallback (see #1094). - VIRTUAL_COLLECTION_EXTENSIONS = ( + # Removed collection-manifest extensions. URLs ending in one of these are + # rejected at parse time with a migration message; the legacy + # `.collection.yml` curated-aggregator format is replaced by `apm.yml` + # with a `dependencies` section (#1094). + REMOVED_COLLECTION_EXTENSIONS = ( ".collection.yml", ".collection.yaml", ) @@ -89,43 +88,34 @@ def is_azure_devops(self) -> bool: def virtual_type(self) -> "VirtualPackageType | None": """Return the type of virtual package, or None if not virtual. - Classification is by extension only -- never by path segment. A path - like ``collections/foo`` is SUBDIRECTORY, NOT COLLECTION; the actual - shape is resolved at fetch time by probing for ``apm.yml`` first and - falling back to ``foo.collection.yml`` (see #1094). COLLECTION is - reserved for URLs that explicitly name a ``.collection.yml`` file. + Classification is by extension only -- never by path segment. + ``.prompt.md``/``.instructions.md``/``.chatmode.md``/``.agent.md`` + is FILE; everything else is SUBDIRECTORY (resolved at fetch time + by probing for ``apm.yml``, ``SKILL.md``, ``plugin.json``, etc). + Paths like ``collections/foo`` (no extension) are SUBDIRECTORY. """ if not self.is_virtual or not self.virtual_path: return None if any(self.virtual_path.endswith(ext) for ext in self.VIRTUAL_FILE_EXTENSIONS): return VirtualPackageType.FILE - if any(self.virtual_path.endswith(ext) for ext in self.VIRTUAL_COLLECTION_EXTENSIONS): - return VirtualPackageType.COLLECTION return VirtualPackageType.SUBDIRECTORY def is_virtual_file(self) -> bool: """Check if this is a virtual file package (individual file).""" return self.virtual_type == VirtualPackageType.FILE - def is_virtual_collection(self) -> bool: - """Check if this is a virtual collection package.""" - return self.virtual_type == VirtualPackageType.COLLECTION - def is_virtual_subdirectory(self) -> bool: """Check if this is a virtual subdirectory package (e.g., Claude Skill). A subdirectory package is a virtual package whose ``virtual_path`` - does not end in a recognized FILE or COLLECTION extension. The - actual on-disk shape is resolved at fetch time -- ``apm.yml``, - ``SKILL.md``, ``plugin.json``, or a sibling ``.collection.yml`` - (legacy fallback for ``/collections/`` paths, see #1094). + does not end in a recognized FILE extension. The actual on-disk + shape is resolved at fetch time -- ``apm.yml``, ``SKILL.md``, + ``plugin.json``, etc. Examples: - ComposioHQ/awesome-claude-skills/brand-guidelines -> True - owner/repo/prompts/file.prompt.md -> False (is_virtual_file) - owner/repo/collections/name -> True (resolved at fetch time) - - owner/repo/collections/name.collection.yml -> False - (is_virtual_collection) """ return self.virtual_type == VirtualPackageType.SUBDIRECTORY @@ -135,7 +125,6 @@ def get_virtual_package_name(self) -> str: For virtual packages, we create a sanitized name from the path: - owner/repo/prompts/code-review.prompt.md -> repo-code-review - owner/repo/collections/project-planning -> repo-project-planning - - owner/repo/collections/project-planning.collection.yml -> repo-project-planning """ if not self.is_virtual or not self.virtual_path: return self.repo_url.split("/")[-1] # Return repo name as fallback @@ -147,10 +136,9 @@ def get_virtual_package_name(self) -> str: # Get the basename without extension path_parts = self.virtual_path.split("/") last = path_parts[-1] - # Strip any recognised virtual extension (file, collection, or none). - # Same naming rule applies regardless of resolved shape: the directory - # name (or file basename) is the user-visible package name. - for ext in self.VIRTUAL_COLLECTION_EXTENSIONS + self.VIRTUAL_FILE_EXTENSIONS: + # Strip any recognised virtual file extension. The directory name + # (or file basename) is the user-visible package name. + for ext in self.VIRTUAL_FILE_EXTENSIONS: if last.endswith(ext): last = last[: -len(ext)] break @@ -645,12 +633,22 @@ def _detect_virtual_package(cls, dependency_str: str): # Security: reject path traversal in virtual path validate_path_segments(virtual_path, context="virtual path") - # Accept any path ending in a recognised virtual extension - # (file or collection-manifest). Reject other dotted final - # segments so typos like `prompts/file.txt` fail fast instead - # of silently mis-classifying as a subdirectory. - recognised_exts = cls.VIRTUAL_FILE_EXTENSIONS + cls.VIRTUAL_COLLECTION_EXTENSIONS - if any(virtual_path.endswith(ext) for ext in recognised_exts): + # Reject removed `.collection.yml` extensions with a clear + # migration message (#1094). Curated dependency aggregators + # are now expressed as `apm.yml` with a `dependencies` block. + if any(virtual_path.endswith(ext) for ext in cls.REMOVED_COLLECTION_EXTENSIONS): + raise ValueError( + f".collection.yml is no longer supported. " + f"Convert '{virtual_path}' to an apm.yml with a " + f"'dependencies' section. " + f"See: https://microsoft.github.io/apm/guides/dependencies/" + ) + + # Accept any path ending in a recognised virtual file + # extension. Reject other dotted final segments so typos like + # `prompts/file.txt` fail fast instead of silently + # mis-classifying as a subdirectory. + if any(virtual_path.endswith(ext) for ext in cls.VIRTUAL_FILE_EXTENSIONS): pass else: last_segment = virtual_path.split("/")[-1] @@ -1030,8 +1028,8 @@ def parse(cls, dependency_str: str) -> "DependencyReference": - user/repo@alias - user/repo#ref@alias - user/repo/path/to/file.prompt.md (virtual file package) - - user/repo/collections/name.collection.yml (virtual collection package) - user/repo/skills/foo (virtual subdirectory package) + - user/repo/collections/foo (virtual subdirectory package) - https://gitlab.com/owner/repo.git (generic HTTPS git URL) - git@gitlab.com:owner/repo.git (SSH git URL) - ssh://git@gitlab.com/owner/repo.git (SSH protocol URL) diff --git a/src/apm_cli/models/dependency/types.py b/src/apm_cli/models/dependency/types.py index 1b8ae0056..7a20e2fca 100644 --- a/src/apm_cli/models/dependency/types.py +++ b/src/apm_cli/models/dependency/types.py @@ -27,7 +27,6 @@ class VirtualPackageType(Enum): """Type of virtual package.""" FILE = "file" # Individual file (*.prompt.md, etc.) - COLLECTION = "collection" # Collection virtual package SUBDIRECTORY = "subdirectory" # Subdirectory package diff --git a/src/apm_cli/models/validation.py b/src/apm_cli/models/validation.py index cc9675a3f..67e7214c6 100644 --- a/src/apm_cli/models/validation.py +++ b/src/apm_cli/models/validation.py @@ -21,15 +21,12 @@ class PackageType(Enum): (presence of apm.yml, SKILL.md, hooks/, plugin.json, etc.). """ - APM_PACKAGE = "apm_package" # Has apm.yml + .apm/ + APM_PACKAGE = "apm_package" # Has apm.yml (.apm/ optional when deps declared) CLAUDE_SKILL = "claude_skill" # Has SKILL.md, no apm.yml HOOK_PACKAGE = "hook_package" # Has hooks/hooks.json, no apm.yml or SKILL.md HYBRID = "hybrid" # Has both apm.yml and SKILL.md (root) MARKETPLACE_PLUGIN = "marketplace_plugin" # Has plugin.json or .claude-plugin/ SKILL_BUNDLE = "skill_bundle" # Has skills//SKILL.md (nested), apm.yml optional - META_PACKAGE = ( - "meta_package" # apm.yml declares deps but no .apm/ -- a curated dep aggregator (#1094) - ) INVALID = "invalid" # None of the above @@ -235,15 +232,11 @@ def detect_package_type( 3. ``CLAUDE_SKILL`` -- root ``SKILL.md`` only (no ``apm.yml``). 4. ``SKILL_BUNDLE`` -- nested ``skills//SKILL.md`` detected; ``apm.yml`` optional; no ``.apm/`` required. - 5. ``APM_PACKAGE`` -- ``apm.yml`` AND ``.apm/`` directory present. - 6. ``META_PACKAGE`` -- ``apm.yml`` only, no ``.apm/`` and no nested - skills, but ``apm.yml`` declares dependencies. The package is a + 5. ``APM_PACKAGE`` -- ``apm.yml`` present. ``.apm/`` is optional: a + dep-only ``apm.yml`` (no ``.apm/`` and no nested skills) is a valid curated aggregator that contributes no own primitives (#1094). - 7. ``INVALID`` -- ``apm.yml`` present but no ``.apm/``, no nested - skills, AND no declared dependencies (helpful error: author - likely needs to add .apm/ or dependencies). - 8. ``HOOK_PACKAGE`` -- ``hooks/*.json`` only, no other signals. - 9. ``INVALID`` -- nothing recognisable. + 6. ``HOOK_PACKAGE`` -- ``hooks/*.json`` only, no other signals. + 7. ``INVALID`` -- nothing recognisable. Returns: A ``(package_type, plugin_json_path)`` tuple. *plugin_json_path* @@ -268,38 +261,37 @@ def detect_package_type( if evidence.nested_skill_dirs: return PackageType.SKILL_BUNDLE, None - # 5. apm.yml + .apm/ -> APM_PACKAGE + # 5. apm.yml present -> APM classification. + # With .apm/ OR declared dependencies, a valid APM_PACKAGE. + # Without either, INVALID (the user committed to "this is an APM + # package" by adding apm.yml; we trust that signal and surface the + # standard "missing .apm/" diagnostic instead of silently falling + # through to a hooks/skill-bundle classification). Dep-only is + # valid as a curated aggregator (#1094). if evidence.has_apm_yml: apm_dir = package_path / APM_DIR - if apm_dir.is_dir(): + if apm_dir.exists() or _apm_yml_declares_dependencies(package_path / APM_YML_FILENAME): return PackageType.APM_PACKAGE, None - # 6. apm.yml + dependencies (no .apm/, no nested skills) -> META_PACKAGE. - # A meta-package is a curated dep aggregator with no own primitives; - # validation passes through to dependency resolution unchanged (#1094). - if _apm_yml_declares_dependencies(package_path / APM_YML_FILENAME): - return PackageType.META_PACKAGE, None - # 7. apm.yml only, nothing useful in it -> INVALID (the original - # error wording still applies: author likely needs `.apm/` or deps). return PackageType.INVALID, None - # 8. hooks/*.json only -> HOOK_PACKAGE + # 6. hooks/*.json only -> HOOK_PACKAGE if evidence.has_hook_json: return PackageType.HOOK_PACKAGE, None - # 9. Nothing recognisable -> INVALID + # 7. Nothing recognisable -> INVALID return PackageType.INVALID, None def _apm_yml_declares_dependencies(apm_yml_path: Path) -> bool: """Return True iff ``apm.yml`` declares at least one dependency. - Used to distinguish a META_PACKAGE (intentional curated aggregator) - from an unfinished APM_PACKAGE (apm.yml authored but `.apm/` not yet - added). Any non-empty ``apm`` or ``mcp`` list under ``dependencies`` + Used by ``_validate_apm_package_with_yml`` to accept a dep-only + ``apm.yml`` (no ``.apm/`` directory) as a valid curated aggregator + (#1094). Any non-empty ``apm`` or ``mcp`` list under ``dependencies`` OR ``devDependencies`` qualifies. Tolerant of malformed YAML / - missing keys: returns False on any parse problem so callers get the - legacy INVALID diagnostic rather than a silent reclassification - (#1094). + missing keys: returns False on any parse problem so callers fall + back to the legacy "missing .apm/" diagnostic instead of silently + accepting a malformed manifest. """ try: from ..utils.yaml_io import load_yaml @@ -313,13 +305,15 @@ def _apm_yml_declares_dependencies(apm_yml_path: Path) -> bool: def _has_listed_deps(block: object) -> bool: if not isinstance(block, dict): return False - # Schema requires `apm` and `mcp` to be lists. Strings, dicts, and - # other truthy non-list values are malformed; treat them as "no - # declared dependencies" so the caller falls through to the legacy - # INVALID diagnostic instead of silently reclassifying as META. + # Schema requires `apm` and `mcp` to be lists of strings or dicts + # (see APMPackage._parse_dependency_dict). Non-list values, or + # lists with no parseable entries, are malformed; treat them as + # "no declared dependencies" so the caller falls through to the + # legacy "missing .apm/" diagnostic instead of silently accepting + # a malformed manifest. for key in ("apm", "mcp"): value = block.get(key) - if isinstance(value, list) and value: + if isinstance(value, list) and any(isinstance(entry, (str, dict)) for entry in value): return True return False @@ -331,15 +325,14 @@ def _has_listed_deps(block: object) -> bool: def validate_apm_package(package_path: Path) -> ValidationResult: """Validate that a directory contains a valid APM package or Claude Skill. - Supports seven package types: - - APM_PACKAGE: Has apm.yml and .apm/ directory + Supports six package types: + - APM_PACKAGE: Has apm.yml (with .apm/ for own primitives, or + dep-only as a curated dependency aggregator -- #1094) - CLAUDE_SKILL: Has SKILL.md but no apm.yml (auto-generates apm.yml) - HOOK_PACKAGE: Has hooks/*.json but no apm.yml or SKILL.md - MARKETPLACE_PLUGIN: Has plugin.json or .claude-plugin/ (synthesizes apm.yml) - HYBRID: Has both apm.yml and root SKILL.md - SKILL_BUNDLE: Has skills//SKILL.md, apm.yml optional - - META_PACKAGE: apm.yml declares deps but no .apm/ -- a curated dep - aggregator with no own primitives (#1094) Args: package_path: Path to the directory to validate @@ -408,12 +401,6 @@ def validate_apm_package(package_path: Path) -> ValidationResult: # Standard APM package or HYBRID validation (has apm.yml) apm_yml_path = package_path / APM_YML_FILENAME - # Meta-packages (#1094): apm.yml is purely a curated dep aggregator - # with no `.apm/` and no own primitives. Validation parses apm.yml and - # returns; transitive resolution is handled downstream. - if result.package_type == PackageType.META_PACKAGE: - return _validate_meta_package(package_path, apm_yml_path, result) - # HYBRID packages: if .apm/ exists, fall through to standard validation # (back-compat for packages that ship both .apm/ primitives AND SKILL.md). # Otherwise validate as a skill bundle with apm.yml metadata. @@ -615,34 +602,6 @@ def _validate_skill_bundle(package_path: Path, result: ValidationResult) -> Vali return result -def _validate_meta_package( - package_path: Path, apm_yml_path: Path, result: ValidationResult -) -> ValidationResult: - """Validate a META_PACKAGE (apm.yml dep aggregator, no `.apm/`). - - A meta-package is a curated dependency set: ``apm.yml`` declares - ``dependencies.apm`` and/or ``dependencies.mcp`` and contributes no - own primitives. There is nothing to integrate at the meta-package - level -- transitive resolution and integration happen on the - declared dependencies as usual (#1094). - - Validation parses ``apm.yml`` to confirm it is structurally well-formed - so downstream resolution has a usable APMPackage handle. ``.apm/`` - intentionally not required: requiring an empty placeholder directory - is the wart this type was added to remove. - """ - from .apm_package import APMPackage - - try: - package = APMPackage.from_apm_yml(apm_yml_path) - except (ValueError, FileNotFoundError) as e: - result.add_error(f"Invalid apm.yml: {e}") - return result - - result.package = package - return result - - def _validate_hybrid_package( package_path: Path, apm_yml_path: Path, result: ValidationResult ) -> ValidationResult: @@ -780,11 +739,16 @@ def _validate_apm_package_with_yml( # Check for .apm directory apm_dir = package_path / APM_DIR if not apm_dir.exists(): + # Dep-only packages (apm.yml with dependencies, no .apm/) are valid + # curated aggregators (#1094). Only fail if there are no dependencies + # either -- that's the original "unfinished package" diagnostic. + if _apm_yml_declares_dependencies(apm_yml_path): + return result result.add_error( f"Missing required directory: {APM_DIR}/ -- " - "an APM package with apm.yml needs a .apm/ directory containing " - "primitives. Alternatively, add a SKILL.md to make this a skill " - "bundle or hybrid package." + "an APM package with apm.yml needs either a .apm/ directory " + "containing primitives, or dependencies declared in apm.yml. " + "Alternatively, add a SKILL.md to make this a skill bundle." ) return result diff --git a/tests/integration/test_collection_install.py b/tests/integration/test_collection_install.py deleted file mode 100644 index 127da4399..000000000 --- a/tests/integration/test_collection_install.py +++ /dev/null @@ -1,245 +0,0 @@ -"""Integration tests for collection virtual package installation.""" - -import shutil # noqa: F401 -import tempfile -from pathlib import Path - -import pytest - -from apm_cli.deps.github_downloader import GitHubPackageDownloader, normalize_collection_path -from apm_cli.models.apm_package import DependencyReference - - -class TestCollectionInstallation: - """Test collection virtual package installation from GitHub.""" - - def test_parse_collection_dependency(self): - """Test parsing a collection dependency reference. - - Per #1094, `/collections/` (no extension) is now SUBDIRECTORY; - the legacy collection shape is resolved at fetch time via the - `.collection.yml` fallback probe. Explicit-extension URLs - remain classified as COLLECTION up-front. - """ - # Implicit form: classified as SUBDIRECTORY, resolved at fetch time. - dep_ref = DependencyReference.parse("owner/test-repo/collections/awesome-copilot") - assert dep_ref.is_virtual is True - assert dep_ref.is_virtual_subdirectory() is True - assert dep_ref.is_virtual_collection() is False - assert dep_ref.is_virtual_file() is False - assert dep_ref.repo_url == "owner/test-repo" - assert dep_ref.virtual_path == "collections/awesome-copilot" - assert dep_ref.get_virtual_package_name() == "test-repo-awesome-copilot" - - # Explicit form: classified as COLLECTION up-front. - dep_ref_explicit = DependencyReference.parse( - "owner/test-repo/collections/awesome-copilot.collection.yml" - ) - assert dep_ref_explicit.is_virtual_collection() is True - assert dep_ref_explicit.get_virtual_package_name() == "test-repo-awesome-copilot" - - def test_parse_collection_with_reference(self): - """Test parsing a collection dependency with git reference.""" - dep_ref = DependencyReference.parse( - "owner/test-repo/collections/project-planning.collection.yml#main" - ) - assert dep_ref.is_virtual is True - assert dep_ref.is_virtual_collection() is True - assert dep_ref.reference == "main" - assert dep_ref.virtual_path == "collections/project-planning.collection.yml" - - @pytest.mark.integration - @pytest.mark.slow - @pytest.mark.skip( - reason="github/awesome-copilot no longer has collections/ directory (deprecated in favor of plugins)" - ) - def test_download_small_collection(self): - """Test downloading a small collection from awesome-copilot. - - This is a real integration test that requires: - - Network access - - GitHub API access - - The github/awesome-copilot repository to be accessible - """ - with tempfile.TemporaryDirectory() as temp_dir: - target_path = Path(temp_dir) / "test-collection" - - downloader = GitHubPackageDownloader() - - # Download the smallest collection (awesome-copilot has 6 items) - package_info = downloader.download_package( - "github/awesome-copilot/collections/awesome-copilot", target_path - ) - - # Verify package was created - assert package_info is not None - assert package_info.package.name == "awesome-copilot-awesome-copilot" - assert "Meta prompts" in package_info.package.description - - # Verify apm.yml was generated - apm_yml = target_path / "apm.yml" - assert apm_yml.exists() - - # Verify .apm directory structure - apm_dir = target_path / ".apm" - assert apm_dir.exists() - - # Verify files were downloaded to correct subdirectories - # The collection should have prompts and chatmodes - prompts_dir = apm_dir / "prompts" - chatmodes_dir = apm_dir / "chatmodes" - - # At least one of these should exist and have files - has_prompts = prompts_dir.exists() and any(prompts_dir.iterdir()) - has_chatmodes = chatmodes_dir.exists() and any(chatmodes_dir.iterdir()) - - assert has_prompts or has_chatmodes, "Collection should have downloaded some files" - - def test_collection_manifest_parsing(self): - """Test parsing a collection manifest.""" - from apm_cli.deps.collection_parser import parse_collection_yml - - manifest_yaml = b""" -id: test-collection -name: Test Collection -description: A test collection for unit testing -tags: [testing, example] -items: - - path: prompts/test-prompt.prompt.md - kind: prompt - - path: instructions/test-instruction.instructions.md - kind: instruction - - path: chatmodes/test-mode.chatmode.md - kind: chat-mode -display: - ordering: alpha - show_badge: true -""" - - manifest = parse_collection_yml(manifest_yaml) - - assert manifest.id == "test-collection" - assert manifest.name == "Test Collection" - assert manifest.description == "A test collection for unit testing" - assert len(manifest.items) == 3 - assert manifest.tags == ["testing", "example"] - - # Check item parsing - assert manifest.items[0].path == "prompts/test-prompt.prompt.md" - assert manifest.items[0].kind == "prompt" - assert manifest.items[0].subdirectory == "prompts" - - assert manifest.items[1].kind == "instruction" - assert manifest.items[1].subdirectory == "instructions" - - assert manifest.items[2].kind == "chat-mode" - assert manifest.items[2].subdirectory == "chatmodes" - - def test_collection_manifest_validation_missing_fields(self): - """Test that collection manifest validation catches missing fields.""" - from apm_cli.deps.collection_parser import parse_collection_yml - - # Missing required field 'description' - invalid_yaml = b""" -id: test -name: Test -items: - - path: test.prompt.md - kind: prompt -""" - - with pytest.raises(ValueError, match="missing required fields"): - parse_collection_yml(invalid_yaml) - - def test_collection_manifest_validation_empty_items(self): - """Test that collection manifest validation catches empty items.""" - from apm_cli.deps.collection_parser import parse_collection_yml - - # Empty items array - invalid_yaml = b""" -id: test -name: Test -description: Test collection -items: [] -""" - - with pytest.raises(ValueError, match="must contain at least one item"): - parse_collection_yml(invalid_yaml) - - def test_collection_manifest_validation_invalid_item(self): - """Test that collection manifest validation catches invalid items.""" - from apm_cli.deps.collection_parser import parse_collection_yml - - # Item missing 'kind' field - invalid_yaml = b""" -id: test -name: Test -description: Test collection -items: - - path: test.prompt.md -""" - - with pytest.raises(ValueError, match="missing required field"): - parse_collection_yml(invalid_yaml) - - def test_parse_collection_with_yml_extension(self): - """Test parsing a collection dependency with .collection.yml extension. - - Regression test for bug where specifying the full extension caused - double-extension paths like 'collections/name.collection.yml.collection.yml'. - """ - dep_ref = DependencyReference.parse( - "copilot/copilot-primitives/collections/markdown-documentation.collection.yml" - ) - - assert dep_ref.is_virtual is True - assert dep_ref.is_virtual_collection() is True - assert dep_ref.repo_url == "copilot/copilot-primitives" - # virtual_path preserves the extension as specified by user - assert dep_ref.virtual_path == "collections/markdown-documentation.collection.yml" - # get_virtual_package_name() should return sanitized name without extension - assert dep_ref.get_virtual_package_name() == "copilot-primitives-markdown-documentation" - - def test_parse_collection_with_yaml_extension(self): - """Test parsing a collection dependency with .collection.yaml extension.""" - dep_ref = DependencyReference.parse("owner/repo/collections/my-collection.collection.yaml") - - assert dep_ref.is_virtual is True - assert dep_ref.is_virtual_collection() is True - assert dep_ref.repo_url == "owner/repo" - assert dep_ref.virtual_path == "collections/my-collection.collection.yaml" - # get_virtual_package_name() should return sanitized name without extension - assert dep_ref.get_virtual_package_name() == "repo-my-collection" - - def test_collection_manifest_path_normalization(self): - """Test that normalize_collection_path correctly strips extensions. - - Regression test: when user specifies .collection.yml in their dependency, - the downloader should NOT append .collection.yml again. - """ - test_cases = [ - # (virtual_path, expected_normalized_path) - ("collections/markdown-documentation", "collections/markdown-documentation"), - ( - "collections/markdown-documentation.collection.yml", - "collections/markdown-documentation", - ), - ( - "collections/markdown-documentation.collection.yaml", - "collections/markdown-documentation", - ), - ("path/to/collections/nested", "path/to/collections/nested"), - ("path/to/collections/nested.collection.yml", "path/to/collections/nested"), - ] - - for virtual_path, expected_base in test_cases: - # Test the actual normalize_collection_path function - normalized = normalize_collection_path(virtual_path) - assert normalized == expected_base, ( - f"normalize_collection_path('{virtual_path}'): expected '{expected_base}', got '{normalized}'" - ) - - # Verify that appending extension gives correct manifest path - manifest_path = f"{normalized}.collection.yml" - expected_manifest = f"{expected_base}.collection.yml" - assert manifest_path == expected_manifest diff --git a/tests/test_apm_package_models.py b/tests/test_apm_package_models.py index a3069667d..f1554b63f 100644 --- a/tests/test_apm_package_models.py +++ b/tests/test_apm_package_models.py @@ -352,7 +352,6 @@ def test_parse_virtual_file_package(self): assert dep.is_virtual is True assert dep.virtual_path == "prompts/code-review.prompt.md" assert dep.is_virtual_file() is True - assert dep.is_virtual_collection() is False assert dep.get_virtual_package_name() == "test-repo-code-review" def test_parse_virtual_file_with_reference(self): @@ -374,50 +373,30 @@ def test_parse_virtual_file_all_extensions(self): assert dep.is_virtual_file() is True assert dep.virtual_path == f"path/to/file{ext}" - def test_parse_virtual_collection_explicit_extension(self): - """Test parsing virtual collection with explicit `.collection.yml`. - - Per #1094, the legacy `/collections/` path heuristic no - longer eagerly classifies as COLLECTION; the actual shape is - resolved at fetch time. Explicit `.collection.yml` URLs remain - classified as COLLECTION up-front. - """ - dep = DependencyReference.parse( - "owner/test-repo/collections/project-planning.collection.yml" - ) - assert dep.repo_url == "owner/test-repo" - assert dep.is_virtual is True - assert dep.virtual_path == "collections/project-planning.collection.yml" - assert dep.is_virtual_file() is False - assert dep.is_virtual_collection() is True - assert dep.get_virtual_package_name() == "test-repo-project-planning" + def test_parse_collection_yml_url_raises_migration_error(self): + """`.collection.yml` URLs are no longer supported (#1094); raise.""" + with pytest.raises(ValueError, match=r"\.collection\.yml is no longer supported"): + DependencyReference.parse("owner/test-repo/collections/project-planning.collection.yml") def test_parse_collections_path_resolves_at_fetch_time(self): """A `/collections/` URL is SUBDIRECTORY now (#1094). - The actual shape (APM package vs legacy collection manifest) is - resolved at fetch time by probing `apm.yml` first, then - `.collection.yml` as a fallback. + After the `.collection.yml` form was removed, the actual shape of a + ``collections/`` path (an APM package or a generic + subdirectory) is resolved at fetch time by probing ``apm.yml``. """ dep = DependencyReference.parse("owner/test-repo/collections/project-planning") assert dep.repo_url == "owner/test-repo" assert dep.is_virtual is True assert dep.virtual_path == "collections/project-planning" assert dep.is_virtual_file() is False - assert dep.is_virtual_collection() is False assert dep.is_virtual_subdirectory() is True - # Naming preserved across the reclassification: same package name - # whether the URL has the explicit extension or not. assert dep.get_virtual_package_name() == "test-repo-project-planning" - def test_parse_virtual_collection_with_reference(self): - """Explicit `.collection.yml` reference still parses as COLLECTION.""" - dep = DependencyReference.parse("owner/test-repo/collections/testing.collection.yml#main") - assert dep.repo_url == "owner/test-repo" - assert dep.is_virtual is True - assert dep.virtual_path == "collections/testing.collection.yml" - assert dep.reference == "main" - assert dep.is_virtual_collection() is True + def test_parse_collection_yml_with_reference_raises_migration_error(self): + """`.collection.yml#ref` is also rejected with the migration error.""" + with pytest.raises(ValueError, match=r"\.collection\.yml is no longer supported"): + DependencyReference.parse("owner/test-repo/collections/testing.collection.yml#main") def test_parse_invalid_virtual_file_extension(self): """Test that invalid file extensions are rejected for virtual files.""" @@ -454,7 +433,6 @@ def test_regular_package_not_virtual(self): assert dep.is_virtual is False assert dep.virtual_path is None assert dep.is_virtual_file() is False - assert dep.is_virtual_collection() is False def test_parse_control_characters_rejected(self): """Test that control characters are rejected.""" diff --git a/tests/test_github_downloader.py b/tests/test_github_downloader.py index bfe7a1ab4..55a355362 100644 --- a/tests/test_github_downloader.py +++ b/tests/test_github_downloader.py @@ -1720,22 +1720,6 @@ def _make_dep_ref(self, virtual_path): ] return dep_ref - def _make_collection_dep_ref(self, virtual_path): - """Helper: build a minimal DependencyReference for a virtual collection.""" - from apm_cli.models.apm_package import DependencyReference - - dep_ref = Mock(spec=DependencyReference) - dep_ref.is_virtual = True - dep_ref.virtual_path = virtual_path - dep_ref.reference = "main" - dep_ref.repo_url = "github/my-org" - dep_ref.get_virtual_package_name.return_value = "my-org-my-collection" - dep_ref.to_github_url.return_value = ( - f"https://github.com/github/my-org/blob/main/{virtual_path}" - ) - dep_ref.is_virtual_collection.return_value = True - return dep_ref - def test_yaml_with_colon_in_description(self, tmp_path): """apm.yml must be valid when the agent description contains a colon.""" import yaml @@ -1808,76 +1792,6 @@ def test_yaml_without_special_characters_still_valid(self, tmp_path): parsed = yaml.safe_load(content) assert parsed["description"] == "A simple agent without special chars" - def test_collection_yaml_with_colon_in_description(self, tmp_path): - """apm.yml for collection packages must be valid when description contains a colon.""" - import yaml - - # A minimal .collection.yml whose description contains ":" - collection_manifest = ( - b"id: my-collection\n" - b"name: My Collection\n" - b"description: 'A collection for tasks: feature development, debugging.'\n" - b"items:\n" - b" - path: agents/my-agent.agent.md\n" - b" kind: agent\n" - ) - agent_file = b"---\nname: My Agent\n---\n## Body\n" - - dep_ref = self._make_collection_dep_ref("collections/my-collection") - target_path = tmp_path / "pkg" - - downloader = GitHubPackageDownloader() - - def _fake_download(dep_ref_arg, path, ref): - if "collection" in path: - return collection_manifest - return agent_file - - with patch.dict(os.environ, {}, clear=True), _CRED_FILL_PATCH: - with patch.object(downloader, "download_raw_file", side_effect=_fake_download): - downloader.download_collection_package(dep_ref, target_path) - - content = (target_path / "apm.yml").read_text(encoding="utf-8") - parsed = yaml.safe_load(content) # must not raise - - assert parsed["description"] == "A collection for tasks: feature development, debugging." - - def test_collection_yaml_with_colon_in_tags(self, tmp_path): - """apm.yml for collection packages must be valid when tags contain a colon.""" - import yaml - - collection_manifest = ( - b"id: tagged-collection\n" - b"name: Tagged\n" - b"description: Normal description\n" - b"tags:\n" - b" - 'scope: engineering'\n" - b" - plain-tag\n" - b"items:\n" - b" - path: agents/my-agent.agent.md\n" - b" kind: agent\n" - ) - agent_file = b"---\nname: My Agent\n---\n## Body\n" - - dep_ref = self._make_collection_dep_ref("collections/tagged-collection") - target_path = tmp_path / "pkg" - - downloader = GitHubPackageDownloader() - - def _fake_download(dep_ref_arg, path, ref): - if "collection" in path: - return collection_manifest - return agent_file - - with patch.dict(os.environ, {}, clear=True), _CRED_FILL_PATCH: - with patch.object(downloader, "download_raw_file", side_effect=_fake_download): - downloader.download_collection_package(dep_ref, target_path) - - content = (target_path / "apm.yml").read_text(encoding="utf-8") - parsed = yaml.safe_load(content) - - assert parsed["tags"] == ["scope: engineering", "plain-tag"] - class TestRefExistsViaLsRemote: """Tests for the ``_ref_exists_via_ls_remote`` two/three-attempt chain. diff --git a/tests/unit/deps/test_github_downloader_validation.py b/tests/unit/deps/test_github_downloader_validation.py index d9f08165c..5a9833674 100644 --- a/tests/unit/deps/test_github_downloader_validation.py +++ b/tests/unit/deps/test_github_downloader_validation.py @@ -694,45 +694,16 @@ def test_explicit_ref_still_activates_git_fallback(self) -> None: class TestSubdirectoryProbeOrder: - """``apm.yml`` is probed before ``.collection.yml`` for SUBDIRECTORY paths. + """``apm.yml`` is probed first for SUBDIRECTORY paths. The legacy ``/collections/`` path heuristic is removed: a path like ``collections/foo`` is now classified SUBDIRECTORY and resolved at fetch - time. ``apm.yml`` must win over the ``.collection.yml`` fallback so a - fixed-but-mis-routed ``collections/foo/apm.yml`` is recognised as an - APM package, not as a missing collection manifest (#1094). + time. ``apm.yml`` is the supported way to express a curated dependency + aggregator (#1094); the ``.collection.yml`` form was removed. """ - def test_explicit_collection_yml_url_probes_path_directly(self) -> None: - """Explicit `.collection.yml` URLs probe the file directly, no double extension. - - Regression: an earlier implementation appended `.collection.yml` to - every COLLECTION-typed vpath, producing - ``foo.collection.yml.collection.yml`` for explicit-extension URLs. - After the extension-only classification (#1094), COLLECTION refs - already carry the extension in vpath -- the probe must use vpath - directly. - """ - downloader = GitHubPackageDownloader() - # Construct an explicit-extension COLLECTION ref via parser so the - # test exercises the full classification cascade. - from apm_cli.models.apm_package import DependencyReference - - dep_ref = DependencyReference.parse("owner/repo/collections/foo.collection.yml#main") - assert dep_ref.is_virtual_collection() is True - - with patch.object(downloader, "download_raw_file") as raw: - raw.return_value = b"id: foo\nname: foo\ndescription: x\nitems: []\n" - ok = gdv.validate_virtual_package_exists(downloader, dep_ref) - - assert ok is True - attempted = [call.args[1] for call in raw.call_args_list] - assert "collections/foo.collection.yml" in attempted - # No double extension. - assert "collections/foo.collection.yml.collection.yml" not in attempted - def test_apm_yml_at_collections_path_short_circuits_collection_probe(self) -> None: - """`/apm.yml` hits first; `.collection.yml` never probed.""" + """`/apm.yml` hits first; subdirectory probe stops there.""" downloader = GitHubPackageDownloader() dep_ref = _make_subdir_dep(vpath="collections/writing", ref="main") @@ -746,126 +717,5 @@ def fake_download(_dr, path, _ref): ok = gdv.validate_virtual_package_exists(downloader, dep_ref) assert ok is True - # Only one HTTP probe should have happened: the apm.yml hit short - # circuits the rest of the probe order. Defensively verify the - # collection probes were never reached. attempted_paths = [call.args[1] for call in raw.call_args_list] assert "collections/writing/apm.yml" in attempted_paths - assert "collections/writing.collection.yml" not in attempted_paths - assert "collections/writing.collection.yaml" not in attempted_paths - - def test_collection_yml_fallback_when_no_apm_yml(self) -> None: - """Legacy `.collection.yml` fallback validates when apm.yml absent.""" - downloader = GitHubPackageDownloader() - dep_ref = _make_subdir_dep(vpath="collections/legacy", ref="main") - - def fake_download(_dr, path, _ref): - if path == "collections/legacy.collection.yml": - return b"description: legacy collection\nitems: []\n" - raise RuntimeError("404") - - with ( - patch.object(downloader, "download_raw_file", side_effect=fake_download), - patch.object(gdv, "_directory_exists_at_ref", return_value=False), - patch.object(gdv, "_ref_exists_via_ls_remote", return_value=(False, None)), - ): - ok = gdv.validate_virtual_package_exists(downloader, dep_ref) - - assert ok is True - - def test_collection_yaml_fallback_when_no_apm_yml(self) -> None: - """The `.collection.yaml` extension is also recognised as a fallback.""" - downloader = GitHubPackageDownloader() - dep_ref = _make_subdir_dep(vpath="collections/legacy", ref="main") - - def fake_download(_dr, path, _ref): - if path == "collections/legacy.collection.yaml": - return b"description: legacy collection\nitems: []\n" - raise RuntimeError("404") - - with ( - patch.object(downloader, "download_raw_file", side_effect=fake_download), - patch.object(gdv, "_directory_exists_at_ref", return_value=False), - patch.object(gdv, "_ref_exists_via_ls_remote", return_value=(False, None)), - ): - ok = gdv.validate_virtual_package_exists(downloader, dep_ref) - - assert ok is True - - -class TestLegacyCollectionFallbackDispatch: - """`_is_legacy_collection_fallback` decides routing for SUBDIRECTORY paths.""" - - def test_apm_yml_present_means_not_a_legacy_collection(self) -> None: - """When `/apm.yml` exists, the dispatcher must NOT redirect to - the collection downloader (apm.yml takes priority).""" - downloader = GitHubPackageDownloader() - dep_ref = _make_subdir_dep(vpath="collections/writing", ref="main") - - def fake_download(_dr, path, _ref): - if path == "collections/writing/apm.yml": - return b"name: writing\nversion: 1.0.0\n" - raise RuntimeError("404") - - with patch.object(downloader, "download_raw_file", side_effect=fake_download): - assert downloader._is_legacy_collection_fallback(dep_ref) is False - - def test_only_collection_yml_means_legacy_collection(self) -> None: - """No `/apm.yml` but `.collection.yml` exists -> legacy collection.""" - downloader = GitHubPackageDownloader() - dep_ref = _make_subdir_dep(vpath="collections/writing", ref="main") - - def fake_download(_dr, path, _ref): - if path == "collections/writing.collection.yml": - return b"description: legacy\nitems: []\n" - raise RuntimeError("404") - - with patch.object(downloader, "download_raw_file", side_effect=fake_download): - assert downloader._is_legacy_collection_fallback(dep_ref) is True - - def test_neither_present_under_collections_path_no_fallback(self) -> None: - """A `collections/` path with no apm.yml and no .collection.yml - means no legacy collection (caller falls through to the regular - subdirectory clone path).""" - downloader = GitHubPackageDownloader() - dep_ref = _make_subdir_dep(vpath="collections/whatever", ref="main") - - with patch.object(downloader, "download_raw_file", side_effect=RuntimeError("404")): - assert downloader._is_legacy_collection_fallback(dep_ref) is False - - def test_only_runs_for_subdirectory_refs(self) -> None: - """Explicit COLLECTION refs route directly; the helper short-circuits.""" - downloader = GitHubPackageDownloader() - dep_ref = DependencyReference( - repo_url="owner/repo", - virtual_path="collections/writing.collection.yml", - is_virtual=True, - reference="main", - ) - # No download_raw_file call should happen at all. - with patch.object(downloader, "download_raw_file") as raw: - assert downloader._is_legacy_collection_fallback(dep_ref) is False - raw.assert_not_called() - - def test_path_outside_collections_short_circuits_without_http(self) -> None: - """Perf hint: SUBDIRECTORY refs without a `collections/` segment - return immediately without any HTTP probe. - - This optimisation prevents skill installs (e.g. ``skills/my-skill``) - from paying 2-3 wasted HTTP probes per install for a legacy - collection fallback they could never trigger. - """ - downloader = GitHubPackageDownloader() - dep_ref = _make_subdir_dep(vpath="skills/my-skill", ref="main") - with patch.object(downloader, "download_raw_file") as raw: - assert downloader._is_legacy_collection_fallback(dep_ref) is False - raw.assert_not_called() - - def test_collections_prefix_path_runs_probes(self) -> None: - """A path STARTING with `collections/` triggers the probes.""" - downloader = GitHubPackageDownloader() - dep_ref = _make_subdir_dep(vpath="collections/foo", ref="main") - with patch.object(downloader, "download_raw_file", side_effect=RuntimeError("404")) as raw: - assert downloader._is_legacy_collection_fallback(dep_ref) is False - # apm.yml probe + 2 collection probes = 3 calls total. - assert raw.call_count == 3 diff --git a/tests/unit/test_ado_path_structure.py b/tests/unit/test_ado_path_structure.py index 2a3bd4298..f21614211 100644 --- a/tests/unit/test_ado_path_structure.py +++ b/tests/unit/test_ado_path_structure.py @@ -452,12 +452,10 @@ class TestADOVirtualPackagePaths: def test_github_virtual_package_uses_2_level_path(self): """Verify GitHub virtual packages install to 2-level path.""" - dep = DependencyReference.parse( - "owner/test-repo/collections/project-planning.collection.yml" - ) + dep = DependencyReference.parse("owner/test-repo/collections/project-planning") assert dep.is_virtual is True - assert dep.is_virtual_collection() is True + assert dep.is_virtual_subdirectory() is True # Simulate install path logic from cli.py for virtual packages repo_parts = dep.repo_url.split("/") @@ -476,12 +474,12 @@ def test_github_virtual_package_uses_2_level_path(self): def test_ado_virtual_collection_uses_3_level_path(self): """Verify ADO virtual collections install to 3-level path.""" dep = DependencyReference.parse( - "dev.azure.com/myorg/myproject/myrepo/collections/my-collection.collection.yml" + "dev.azure.com/myorg/myproject/myrepo/collections/my-collection" ) assert dep.is_azure_devops() is True assert dep.is_virtual is True - assert dep.is_virtual_collection() is True + assert dep.is_virtual_subdirectory() is True assert dep.repo_url == "myorg/myproject/myrepo" # Simulate install path logic from cli.py for virtual packages @@ -526,15 +524,14 @@ def test_ado_virtual_file_uses_3_level_path(self): def test_ado_collection_with_git_segment(self): """Verify ADO collections with _git segment work correctly.""" dep = DependencyReference.parse( - "dev.azure.com/myorg/myproject/_git/copilot-instructions/collections/" - "csharp-ddd.collection.yml" + "dev.azure.com/myorg/myproject/_git/copilot-instructions/collections/csharp-ddd" ) assert dep.is_azure_devops() is True assert dep.is_virtual is True - assert dep.is_virtual_collection() is True + assert dep.is_virtual_subdirectory() is True assert dep.repo_url == "myorg/myproject/copilot-instructions" - assert dep.virtual_path == "collections/csharp-ddd.collection.yml" + assert dep.virtual_path == "collections/csharp-ddd" # Verify correct install path repo_parts = dep.repo_url.split("/") @@ -561,13 +558,13 @@ def test_ado_collection_missing_repo_name_parsed_incorrectly(self): assert dep.repo_url == "myorg/myproject/collections" assert dep.virtual_path == "my-collection" - # This causes is_virtual_collection() to return False - # because virtual_path doesn't contain 'collections/' - assert dep.is_virtual_collection() is False + # The shape no longer matches the SUBDIRECTORY heuristic + # (virtual_path is `my-collection`, no extension) so it is treated + # as a subdirectory virtual ref under repo `myorg/myproject/collections` + # -- the install will fail downstream with a 'repo not found' error. + assert dep.is_virtual_subdirectory() is True assert dep.is_virtual_file() is False - # This is why the user gets "Unknown virtual package type" error - class TestADOPruneCommand: """Test that prune command handles ADO 3-level paths correctly.""" diff --git a/tests/unit/test_artifactory_support.py b/tests/unit/test_artifactory_support.py index a653dc1a7..67919306b 100644 --- a/tests/unit/test_artifactory_support.py +++ b/tests/unit/test_artifactory_support.py @@ -871,13 +871,6 @@ def test_virtual_file_errors_without_base_url(self): with pytest.raises(RuntimeError, match="PROXY_REGISTRY_ONLY is set"): dl.download_package("owner/repo/prompts/deploy.prompt.md", Path("/tmp/test-pkg")) - def test_virtual_collection_errors_without_base_url(self): - """PROXY_REGISTRY_ONLY without PROXY_REGISTRY_URL raises for virtual collection packages.""" - with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): - dl = GitHubPackageDownloader() - with pytest.raises(RuntimeError, match="PROXY_REGISTRY_ONLY is set"): - dl.download_package("owner/repo/collections/my-collection", Path("/tmp/test-pkg")) - def test_virtual_subdirectory_errors_without_base_url(self): """PROXY_REGISTRY_ONLY without PROXY_REGISTRY_URL raises for virtual subdirectory packages.""" with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): @@ -898,18 +891,19 @@ def test_explicit_artifactory_fqdn_virtual_file_passes(self): with patch.object(dl, "download_virtual_file_package", return_value=MagicMock()): dl.download_package(dep, Path("/tmp/test-pkg")) - def test_explicit_artifactory_fqdn_virtual_collection_passes(self): - """Explicit Artifactory FQDN on virtual collection dep is NOT blocked by PROXY_REGISTRY_ONLY.""" + def test_explicit_artifactory_fqdn_virtual_subdirectory_passes(self): + """Explicit Artifactory FQDN on virtual subdirectory dep is NOT blocked by PROXY_REGISTRY_ONLY.""" with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): dl = GitHubPackageDownloader() dep = DependencyReference.parse( - "art.example.com/artifactory/github/owner/repo/collections/" - "my-collection.collection.yml" + "art.example.com/artifactory/github/owner/repo/collections/my-collection" ) assert dep.is_artifactory() - assert dep.is_virtual_collection() + assert dep.is_virtual_subdirectory() # Should not raise - explicit Artifactory FQDN bypasses the guard - with patch.object(dl, "download_collection_package", return_value=MagicMock()): + with patch.object( + dl, "_download_subdirectory_from_artifactory", return_value=MagicMock() + ): dl.download_package(dep, Path("/tmp/test-pkg")) def test_proxy_registry_only_is_canonical(self): diff --git a/tests/unit/test_collection_migration_error.py b/tests/unit/test_collection_migration_error.py new file mode 100644 index 000000000..afddbcf6b --- /dev/null +++ b/tests/unit/test_collection_migration_error.py @@ -0,0 +1,51 @@ +"""Regression-trap tests for the .collection.yml -> apm.yml migration (#1094). + +The `.collection.yml` curated-aggregator format was removed in favor of +dep-only ``apm.yml``. Any URL still ending in `.collection.yml` or +`.collection.yaml` MUST raise a clear migration ``ValueError`` at parse +time so users know exactly what to change. +""" + +import pytest + +from src.apm_cli.models.dependency.reference import DependencyReference + + +class TestCollectionMigrationError: + """Parsing `.collection.yml` URLs raises a migration ValueError.""" + + def test_collection_yml_url_raises(self): + with pytest.raises(ValueError, match=r"\.collection\.yml is no longer supported"): + DependencyReference.parse("owner/repo/collections/writing.collection.yml") + + def test_collection_yaml_url_raises(self): + with pytest.raises(ValueError, match=r"\.collection\.yml is no longer supported"): + DependencyReference.parse("owner/repo/collections/writing.collection.yaml") + + def test_collection_yml_with_ref_raises(self): + with pytest.raises(ValueError, match=r"\.collection\.yml is no longer supported"): + DependencyReference.parse("owner/repo/collections/writing.collection.yml#v1.0.0") + + def test_ado_collection_yml_raises(self): + """ADO-style URLs are also rejected.""" + with pytest.raises(ValueError, match=r"\.collection\.yml is no longer supported"): + DependencyReference.parse( + "dev.azure.com/org/project/_git/repo/collections/writing.collection.yml" + ) + + def test_error_message_points_to_apm_yml_migration(self): + """The error message tells users exactly how to migrate.""" + with pytest.raises(ValueError) as exc_info: + DependencyReference.parse("owner/repo/collections/writing.collection.yml") + msg = str(exc_info.value) + assert "apm.yml" in msg + assert "dependencies" in msg + assert "microsoft.github.io/apm" in msg + + def test_collections_path_without_extension_still_parses(self): + """A `collections/` URL with NO `.collection.yml` extension is + a valid SUBDIRECTORY reference (no migration error). + """ + ref = DependencyReference.parse("owner/repo/collections/writing") + assert ref.is_virtual_subdirectory() + assert ref.virtual_path == "collections/writing" diff --git a/tests/unit/test_meta_package.py b/tests/unit/test_dep_only_package.py similarity index 59% rename from tests/unit/test_meta_package.py rename to tests/unit/test_dep_only_package.py index 67ef9f1d2..71225a221 100644 --- a/tests/unit/test_meta_package.py +++ b/tests/unit/test_dep_only_package.py @@ -1,10 +1,10 @@ -"""Unit tests for META_PACKAGE detection and validation (#1094). +"""Unit tests for dep-only APM_PACKAGE detection and validation (#1094). -A meta-package is a curated dependency aggregator: ``apm.yml`` declares +A dep-only package is a curated dependency aggregator: ``apm.yml`` declares ``dependencies.apm`` and/or ``dependencies.mcp`` and contributes no own primitives (no ``.apm/``, no ``SKILL.md``, no nested skills). This shape existed in practice as a workaround that required an empty ``.apm/.gitkeep``; -issue #1094 promoted it to a first-class type so users no longer need to +issue #1094 collapsed it into APM_PACKAGE so users no longer need to commit a placeholder directory just to satisfy the structural check. """ @@ -17,14 +17,14 @@ from src.apm_cli.models.validation import detect_package_type -class TestMetaPackageDetection: - """Detection cascade: META_PACKAGE classification.""" +class TestDepOnlyPackageDetection: + """Detection cascade: dep-only apm.yml -> APM_PACKAGE.""" def _write_apm_yml(self, tmp_path: Path, body: str) -> None: (tmp_path / "apm.yml").write_text(body) - def test_apm_yml_with_apm_deps_detected_as_meta(self, tmp_path): - """apm.yml + non-empty dependencies.apm + no .apm/ -> META_PACKAGE.""" + def test_apm_yml_with_apm_deps_detected_as_apm_package(self, tmp_path): + """apm.yml + non-empty dependencies.apm + no .apm/ -> APM_PACKAGE.""" self._write_apm_yml( tmp_path, "name: writing\n" @@ -35,19 +35,19 @@ def test_apm_yml_with_apm_deps_detected_as_meta(self, tmp_path): " mcp: []\n", ) pkg_type, _ = detect_package_type(tmp_path) - assert pkg_type == PackageType.META_PACKAGE + assert pkg_type == PackageType.APM_PACKAGE - def test_apm_yml_with_dev_deps_only_detected_as_meta(self, tmp_path): - """A dev-only dep aggregator is still a meta-package.""" + def test_apm_yml_with_dev_deps_only_detected_as_apm_package(self, tmp_path): + """A dev-only dep aggregator is still an APM_PACKAGE.""" self._write_apm_yml( tmp_path, "name: dev-bundle\nversion: 1.0.0\ndevDependencies:\n apm:\n - some/dev-tool\n", ) pkg_type, _ = detect_package_type(tmp_path) - assert pkg_type == PackageType.META_PACKAGE + assert pkg_type == PackageType.APM_PACKAGE - def test_apm_yml_with_mcp_deps_only_detected_as_meta(self, tmp_path): - """apm.yml with only mcp deps and no .apm/ still meta.""" + def test_apm_yml_with_mcp_deps_only_detected_as_apm_package(self, tmp_path): + """apm.yml with only mcp deps and no .apm/ still APM_PACKAGE.""" self._write_apm_yml( tmp_path, "name: mcp-bundle\n" @@ -58,7 +58,7 @@ def test_apm_yml_with_mcp_deps_only_detected_as_meta(self, tmp_path): " - some/mcp-server\n", ) pkg_type, _ = detect_package_type(tmp_path) - assert pkg_type == PackageType.META_PACKAGE + assert pkg_type == PackageType.APM_PACKAGE def test_apm_yml_no_deps_no_apm_dir_still_invalid(self, tmp_path): """apm.yml with no deps and no .apm/ stays INVALID (the warning case).""" @@ -85,8 +85,8 @@ def test_apm_yml_with_apm_dir_still_apm_package(self, tmp_path): pkg_type, _ = detect_package_type(tmp_path) assert pkg_type == PackageType.APM_PACKAGE - def test_apm_yml_meta_with_skill_bundle_still_skill_bundle(self, tmp_path): - """Nested skills//SKILL.md takes priority over META_PACKAGE.""" + def test_apm_yml_with_skill_bundle_still_skill_bundle(self, tmp_path): + """Nested skills//SKILL.md takes priority over a dep-only apm.yml.""" self._write_apm_yml( tmp_path, "name: bundle\nversion: 1.0.0\ndependencies:\n apm:\n - owner/repo/skills/foo\n", @@ -112,14 +112,12 @@ def test_apm_yml_with_skill_md_root_still_hybrid(self, tmp_path): assert pkg_type == PackageType.HYBRID def test_malformed_apm_yml_treated_as_invalid(self, tmp_path): - """Tolerant of unparseable apm.yml: INVALID, not META_PACKAGE.""" + """Tolerant of unparseable apm.yml: INVALID (no .apm/, no nested skills).""" self._write_apm_yml(tmp_path, "name: bad\nversion: 1.0.0\ndependencies: not-a-dict\n") pkg_type, _ = detect_package_type(tmp_path) - # No .apm/, no nested skills, deps unparseable -> falls through to INVALID - # so the user sees the standard "missing .apm/" diagnostic. assert pkg_type == PackageType.INVALID - def test_apm_string_value_is_invalid_not_meta(self, tmp_path): + def test_apm_string_value_is_invalid(self, tmp_path): """Schema requires apm to be a list; a string value is malformed -> INVALID.""" self._write_apm_yml( tmp_path, @@ -128,7 +126,7 @@ def test_apm_string_value_is_invalid_not_meta(self, tmp_path): pkg_type, _ = detect_package_type(tmp_path) assert pkg_type == PackageType.INVALID - def test_apm_dict_value_is_invalid_not_meta(self, tmp_path): + def test_apm_dict_value_is_invalid(self, tmp_path): """Schema requires apm to be a list; a dict value is malformed -> INVALID.""" self._write_apm_yml( tmp_path, @@ -137,12 +135,59 @@ def test_apm_dict_value_is_invalid_not_meta(self, tmp_path): pkg_type, _ = detect_package_type(tmp_path) assert pkg_type == PackageType.INVALID + def test_apm_list_with_only_non_parseable_entries_is_invalid(self, tmp_path): + """A list of non-str/non-dict entries (e.g., bare integers) is malformed -> INVALID. + + Regression trap (#1097 panel review): the original guard accepted any + truthy list; that meant `apm: [123]` would parse as an empty package + with no real deps, defeating the safety check the cascade is supposed + to enforce. + """ + self._write_apm_yml( + tmp_path, + "name: malformed\nversion: 1.0.0\ndependencies:\n apm:\n - 123\n mcp: []\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.INVALID + + def test_mcp_list_with_only_non_parseable_entries_is_invalid(self, tmp_path): + """Same regression trap on the `mcp` list.""" + self._write_apm_yml( + tmp_path, + "name: malformed\nversion: 1.0.0\ndependencies:\n apm: []\n mcp:\n - 42\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.INVALID + + def test_dev_deps_list_with_only_non_parseable_entries_is_invalid(self, tmp_path): + """Same regression trap on `devDependencies`.""" + self._write_apm_yml( + tmp_path, + "name: malformed\nversion: 1.0.0\ndevDependencies:\n apm:\n - true\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.INVALID + + def test_apm_list_with_mix_of_parseable_and_garbage_is_apm_package(self, tmp_path): + """A list containing at least one parseable entry is still APM_PACKAGE. + + This mirrors the parser's tolerant behavior (it silently drops + non-str/non-dict entries) -- partial garbage doesn't void the + package, but pure garbage does. + """ + self._write_apm_yml( + tmp_path, + "name: ok\nversion: 1.0.0\ndependencies:\n apm:\n - 123\n - owner/repo\n", + ) + pkg_type, _ = detect_package_type(tmp_path) + assert pkg_type == PackageType.APM_PACKAGE + -class TestMetaPackageValidation: - """Full validate_apm_package: META_PACKAGE passes cleanly.""" +class TestDepOnlyPackageValidation: + """Full validate_apm_package: dep-only APM_PACKAGE passes cleanly.""" - def test_meta_package_passes_validation(self, tmp_path): - """META_PACKAGE validation succeeds without `.apm/`.""" + def test_dep_only_package_passes_validation(self, tmp_path): + """Dep-only APM_PACKAGE validation succeeds without `.apm/`.""" (tmp_path / "apm.yml").write_text( "name: writing\n" "version: 1.0.0\n" @@ -151,15 +196,15 @@ def test_meta_package_passes_validation(self, tmp_path): ) result = validate_apm_package(tmp_path) assert result.is_valid is True - assert result.package_type == PackageType.META_PACKAGE + assert result.package_type == PackageType.APM_PACKAGE assert result.package is not None assert result.package.name == "writing" assert result.package.version == "1.0.0" - # Validation does not require `.apm/` for META_PACKAGE. + # Validation does not require `.apm/` when dependencies are declared. assert not (tmp_path / ".apm").exists() - def test_meta_package_no_missing_apm_dir_error(self, tmp_path): - """The legacy `missing .apm/` error must NOT fire for META_PACKAGE.""" + def test_dep_only_package_no_missing_apm_dir_error(self, tmp_path): + """The legacy `missing .apm/` error must NOT fire for dep-only APM_PACKAGE.""" (tmp_path / "apm.yml").write_text( "name: writing\nversion: 1.0.0\ndependencies:\n apm:\n - owner/repo/skills/foo\n", ) @@ -168,8 +213,8 @@ def test_meta_package_no_missing_apm_dir_error(self, tmp_path): assert "missing the required .apm/ directory" not in err assert "Missing required directory: .apm/" not in err - def test_invalid_apm_yml_still_errors_for_meta(self, tmp_path): - """A META_PACKAGE with malformed apm.yml structure surfaces the parse error.""" + def test_invalid_apm_yml_still_errors(self, tmp_path): + """A dep-only APM_PACKAGE with malformed apm.yml surfaces the parse error.""" # Pass detect_package_type by declaring deps, but break the from_apm_yml # parser by omitting the required `version` field. (tmp_path / "apm.yml").write_text( diff --git a/tests/unit/test_generic_git_urls.py b/tests/unit/test_generic_git_urls.py index 9ac79bcce..5ae440c84 100644 --- a/tests/unit/test_generic_git_urls.py +++ b/tests/unit/test_generic_git_urls.py @@ -454,15 +454,14 @@ def test_gitlab_virtual_file(self): assert dep.is_virtual is True assert dep.is_virtual_file() is True - def test_bitbucket_virtual_collection(self): - dep = DependencyReference.parse( - "bitbucket.org/team/rules/collections/security.collection.yml" - ) - assert dep.host == "bitbucket.org" - assert dep.repo_url == "team/rules" - assert dep.virtual_path == "collections/security.collection.yml" - assert dep.is_virtual is True - assert dep.is_virtual_collection() is True + def test_bitbucket_collection_yml_url_raises(self): + """`.collection.yml` URLs raise migration error on generic hosts too.""" + import pytest + + with pytest.raises(ValueError, match=r"\.collection\.yml is no longer supported"): + DependencyReference.parse( + "bitbucket.org/team/rules/collections/security.collection.yml" + ) def test_self_hosted_virtual_subdirectory(self): """Without virtual indicators, all segments are repo path on generic hosts. diff --git a/tests/unit/test_package_identity.py b/tests/unit/test_package_identity.py index 5f1f21f29..aea1143a7 100644 --- a/tests/unit/test_package_identity.py +++ b/tests/unit/test_package_identity.py @@ -73,7 +73,7 @@ def test_virtual_file_package(self): ) def test_virtual_collection_package(self): - """Virtual collection includes full path.""" + """Virtual collection (subdirectory) includes full path.""" dep = DependencyReference.parse("owner/test-repo/collections/azure-cloud-development") assert ( dep.get_canonical_dependency_string() @@ -136,37 +136,13 @@ def test_virtual_file_package(self): expected = apm_modules / "owner" / "test-repo-code-review" assert dep.get_install_path(apm_modules) == expected - def test_virtual_collection_package(self): - """Explicit `.collection.yml` URL: apm_modules/owner/. - - Per #1094, only URLs ending in `.collection.yml`/`.yaml` are - eagerly classified as COLLECTION (and use the flattened install - path). Implicit `collections/` URLs are SUBDIRECTORY and - use the natural path layout (covered separately below). - """ - dep = DependencyReference.parse( - "owner/test-repo/collections/azure-cloud-development.collection.yml" - ) - apm_modules = Path("/project/apm_modules") - - # Virtual package name: test-repo-azure-cloud-development (flattened) - expected = apm_modules / "owner" / "test-repo-azure-cloud-development" - assert dep.get_install_path(apm_modules) == expected - - def test_virtual_collection_with_reference(self): - """Reference does not affect virtual package install path.""" - dep = DependencyReference.parse("owner/test-repo/collections/testing.collection.yml#main") - apm_modules = Path("/project/apm_modules") - - expected = apm_modules / "owner" / "test-repo-testing" - assert dep.get_install_path(apm_modules) == expected - def test_collections_path_subdirectory_uses_natural_layout(self): - """`/collections/` (no extension) is SUBDIRECTORY (#1094). + """`/collections/` is SUBDIRECTORY (#1094). SUBDIRECTORY install paths mirror the repo path so an actual `collections//apm.yml` package lives at the natural location - under apm_modules/, instead of the flattened collection layout. + under apm_modules/. The legacy flattened layout (used by the + removed `.collection.yml` form) is gone. """ dep = DependencyReference.parse("owner/test-repo/collections/azure-cloud-development") apm_modules = Path("/project/apm_modules") @@ -191,15 +167,14 @@ def test_ado_virtual_package(self): expected = apm_modules / "myorg" / "myproject" / "myrepo-test" assert dep.get_install_path(apm_modules) == expected - def test_ado_virtual_collection(self): - """ADO explicit `.collection.yml` URL: apm_modules/org/project/.""" + def test_ado_virtual_collection_subdirectory(self): + """ADO `/collections/` is SUBDIRECTORY: natural-layout install path.""" dep = DependencyReference.parse( - "dev.azure.com/myorg/myproject/myrepo/collections/my-collection.collection.yml" + "dev.azure.com/myorg/myproject/myrepo/collections/my-collection" ) apm_modules = Path("/project/apm_modules") - # Virtual package name: myrepo-my-collection (flattened) - expected = apm_modules / "myorg" / "myproject" / "myrepo-my-collection" + expected = apm_modules / "myorg" / "myproject" / "myrepo" / "collections" / "my-collection" assert dep.get_install_path(apm_modules) == expected def test_relative_apm_modules_path(self): @@ -215,7 +190,7 @@ class TestInstallPathConsistency: def test_consistency_with_get_virtual_package_name(self): """Install path's last segment equals get_virtual_package_name() for - flattened-layout virtual refs (FILE / explicit COLLECTION). + flattened-layout virtual refs (FILE). SUBDIRECTORY refs use a natural-layout install path that mirrors the repo structure, so the last-segment invariant does not hold @@ -224,7 +199,6 @@ def test_consistency_with_get_virtual_package_name(self): """ test_cases = [ "owner/test-repo/prompts/code-review.prompt.md", - "owner/test-repo/collections/azure-cloud-development.collection.yml", "owner/repo/agents/security.agent.md", "user/pkg/instructions/coding.instructions.md", ] @@ -268,27 +242,20 @@ def test_regular_package_same_owner(self): class TestUninstallScenarios: """Test scenarios that were broken before the fix.""" - def test_uninstall_virtual_collection_finds_correct_path(self): - """Explicit `.collection.yml` URL still installs to the flattened - (collection) layout so uninstall logic targeting that layout works. + def test_uninstall_virtual_collection_subdirectory_path(self): + """`/collections/` is SUBDIRECTORY: natural-layout install path. - Implicit `collections/` URLs are SUBDIRECTORY (#1094) and use - the natural-layout path -- a separate flow with its own uninstall - logic, exercised by the SUBDIRECTORY tests. + Uninstall logic for SUBDIRECTORY collections targets the natural + path under apm_modules/, mirroring the repo structure. """ - dep_str = "owner/test-repo/collections/azure-cloud-development.collection.yml" + dep_str = "owner/test-repo/collections/azure-cloud-development" dep = DependencyReference.parse(dep_str) apm_modules = Path("apm_modules") install_path = dep.get_install_path(apm_modules) - # Should be owner/test-repo-azure-cloud-development - # NOT owner/test-repo/collections/azure-cloud-development - assert install_path == apm_modules / "owner" / "test-repo-azure-cloud-development" - - # The wrong path (from raw path segments) would be: - wrong_path = apm_modules / "owner" / "test-repo" / "collections" / "azure-cloud-development" - assert install_path != wrong_path + expected = apm_modules / "owner" / "test-repo" / "collections" / "azure-cloud-development" + assert install_path == expected def test_uninstall_virtual_file_finds_correct_path(self): """Uninstalling virtual file should find owner/virtual-pkg-name.""" diff --git a/tests/unit/test_script_runner.py b/tests/unit/test_script_runner.py index a8ccf326d..405e07d39 100644 --- a/tests/unit/test_script_runner.py +++ b/tests/unit/test_script_runner.py @@ -601,36 +601,6 @@ def test_auto_install_virtual_package_file_success( assert result is True mock_downloader.download_virtual_file_package.assert_called_once() - @patch("apm_cli.deps.github_downloader.GitHubPackageDownloader") - @patch("apm_cli.core.script_runner.Path.mkdir") - @patch("apm_cli.core.script_runner.Path.exists") - def test_auto_install_virtual_package_collection_success( - self, mock_exists, mock_mkdir, mock_downloader_class - ): - """Test successful auto-install of virtual collection package.""" - # Setup mocks - mock_exists.return_value = False # Package not already installed - mock_downloader = MagicMock() - mock_downloader_class.return_value = mock_downloader - - # Mock package info - mock_package = MagicMock() - mock_package.name = "test-repo-project-planning" - mock_package.version = "1.0.0" - mock_package_info = MagicMock() - mock_package_info.package = mock_package - mock_downloader.download_virtual_collection_package.return_value = mock_package_info - - # Test auto-install - # Use the explicit `.collection.yml` URL form so the dep is classified - # as COLLECTION up-front (per #1094, the implicit `collections/` - # form is now SUBDIRECTORY and resolved at fetch time). - ref = "owner/test-repo/collections/project-planning.collection.yml" - result = self.script_runner._auto_install_virtual_package(ref) - - assert result is True - mock_downloader.download_virtual_collection_package.assert_called_once() - @patch("apm_cli.core.script_runner.Path.exists") def test_auto_install_virtual_package_already_installed(self, mock_exists): """Test auto-install skips when package already installed.""" From 65e8a7967b09247563a851efd49f0929a88b746f Mon Sep 17 00:00:00 2001 From: Copilot Date: Sat, 2 May 2026 11:10:03 +0200 Subject: [PATCH 4/5] test(deps): close test-coverage gaps for #1094 rework Per test-coverage-expert review: add the missing integration + CLI seam tests for the dep-only apm.yml fix and .collection.yml migration error. - tests/integration/test_apm_dependencies.py: test_dep_only_project_installs_dependencies_without_dot_apm -- end-to-end proof against real microsoft/apm-sample-package that a root project with declared deps but NO .apm/ resolves, downloads, and integrates without the .gitkeep workaround. Closes the #1094 user promise at the install-pipeline seam (unit tests only covered the detection layer). - tests/unit/test_collection_migration_error.py: TestCollectionMigrationErrorPropagation -- 2 tests asserting the parse-time ValueError survives the re-wrap inside APMPackage._parse_dependency_dict for both dependencies.apm and devDependencies.apm. The actionable migration text reaches the install caller intact. - tests/unit/test_install_command.py: test_install_collection_yml_argument_surfaces_migration_message -- CLI-argument seam: `apm install owner/repo/.../foo.collection.yml` routes the parse-time ValueError through _resolve_package_references -> invalid_outcomes -> "All packages failed validation" with the migration text in the user-visible output. Verified locally: all 3 new tests pass (integration test runs against real GitHub). Full sweep: 7516 passed, 2 skipped, 3 pre-existing env failures unrelated to this diff. Co-authored-by: xuyuanhao Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/integration/test_apm_dependencies.py | 40 +++++++++++++++ tests/unit/test_collection_migration_error.py | 50 +++++++++++++++++++ tests/unit/test_install_command.py | 21 ++++++++ 3 files changed, 111 insertions(+) diff --git a/tests/integration/test_apm_dependencies.py b/tests/integration/test_apm_dependencies.py index 7d93cded9..fcadd2777 100644 --- a/tests/integration/test_apm_dependencies.py +++ b/tests/integration/test_apm_dependencies.py @@ -73,6 +73,46 @@ def create_apm_yml(self, dependencies: list[str] = None, **kwargs): # noqa: RUF return config + @pytest.mark.integration + def test_dep_only_project_installs_dependencies_without_dot_apm(self): + """Regression-trap for #1094: a dep-only `apm.yml` (no `.apm/` on the + ROOT project) must resolve, download, and integrate transitive + dependencies end-to-end without requiring a `.gitkeep` placeholder. + + Before the fix, ``_validate_apm_package_with_yml`` rejected this + shape and the install pipeline never even reached the resolver. + After the fix, the root project is classified APM_PACKAGE on the + strength of its declared deps alone. + """ + # Create dep-only apm.yml with NO .apm/ directory on the root. + self.create_apm_yml(dependencies=["microsoft/apm-sample-package"]) + assert not (self.test_dir / ".apm").exists(), ( + "precondition: root project must be dep-only (no .apm/)" + ) + + # Load via the same entry point the install pipeline uses; this + # would have raised "missing .apm/ directory" before the fix. + project_package = APMPackage.from_apm_yml(self.apm_yml_path) + dependencies = project_package.get_apm_dependencies() + assert len(dependencies) == 1 + assert dependencies[0].repo_url == "microsoft/apm-sample-package" + + # Real download proves the install pipeline wires up correctly + # for a dep-only root. + downloader = GitHubPackageDownloader() + apm_modules_dir = self.test_dir / "apm_modules" + apm_modules_dir.mkdir() + package_dir = apm_modules_dir / "microsoft" / "apm-sample-package" + result = downloader.download_package(str(dependencies[0]), package_dir) + + assert package_dir.exists() + assert (package_dir / "apm.yml").exists() + assert (package_dir / ".apm").exists() + assert result.package.name == "apm-sample-package" + # Root remains dep-only after install -- we did NOT create .apm/ + # as a side effect. + assert not (self.test_dir / ".apm").exists() + @pytest.mark.integration def test_single_dependency_installation_sample_package(self): """Test installation of single dependency: apm-sample-package.""" diff --git a/tests/unit/test_collection_migration_error.py b/tests/unit/test_collection_migration_error.py index afddbcf6b..0b1a31d48 100644 --- a/tests/unit/test_collection_migration_error.py +++ b/tests/unit/test_collection_migration_error.py @@ -49,3 +49,53 @@ def test_collections_path_without_extension_still_parses(self): ref = DependencyReference.parse("owner/repo/collections/writing") assert ref.is_virtual_subdirectory() assert ref.virtual_path == "collections/writing" + + +class TestCollectionMigrationErrorPropagation: + """Migration error survives the two-hop wrap from APMPackage.from_apm_yml. + + The unit tests above prove ``DependencyReference.parse()`` raises + correctly. The install pipeline calls ``APMPackage.from_apm_yml()`` + which calls ``_parse_dependency_dict()`` which catches the + ``ValueError`` and re-raises with a prefix. This regression-trap + proves the actionable migration text survives that re-wrap. + """ + + def test_collection_yml_in_apm_yml_surfaces_migration_message(self, tmp_path): + from src.apm_cli.models.apm_package import APMPackage, clear_apm_yml_cache + + clear_apm_yml_cache() + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test-project\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - owner/repo/collections/writing.collection.yml\n" + ) + with pytest.raises(ValueError) as exc_info: + APMPackage.from_apm_yml(apm_yml) + msg = str(exc_info.value) + assert ".collection.yml is no longer supported" in msg + assert "apm.yml" in msg + assert "dependencies" in msg + assert "microsoft.github.io/apm" in msg + assert "Invalid APM dependency" in msg + + def test_collection_yml_in_dev_dependencies_surfaces_migration_message(self, tmp_path): + from src.apm_cli.models.apm_package import APMPackage, clear_apm_yml_cache + + clear_apm_yml_cache() + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test-project\n" + "version: 1.0.0\n" + "devDependencies:\n" + " apm:\n" + " - owner/repo/collections/writing.collection.yaml\n" + ) + with pytest.raises(ValueError) as exc_info: + APMPackage.from_apm_yml(apm_yml) + msg = str(exc_info.value) + assert ".collection.yml is no longer supported" in msg + assert "Invalid dev APM dependency" in msg diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 80d7a1bfd..df1a960f4 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -227,6 +227,27 @@ def test_install_invalid_package_format_with_no_apm_yml(self, mock_validate): assert Path("apm.yml").exists() assert "invalid format" in result.output + @patch("apm_cli.commands.install._validate_package_exists") + def test_install_collection_yml_argument_surfaces_migration_message(self, mock_validate): + """`apm install owner/repo/.../foo.collection.yml` (CLI arg, not in + apm.yml) MUST surface the migration ValueError end-to-end. + + Regression-trap for #1094 rework: the parse-time ValueError from + ``DependencyReference.parse()`` flows through + ``_resolve_package_references`` -> ``invalid_outcomes`` -> + validation summary. If a future refactor swallows this, users + would silently see "package not found" instead of the actionable + migration text. + """ + with self._chdir_tmp(): + result = self.runner.invoke( + cli, ["install", "owner/repo/collections/writing.collection.yml"] + ) + assert ".collection.yml is no" in result.output # text wraps in CLI + assert "longer supported" in result.output + assert "apm.yml" in result.output + assert "All packages failed validation" in result.output + @patch("apm_cli.commands.install._validate_package_exists") @patch("apm_cli.commands.install.APM_DEPS_AVAILABLE", True) @patch("apm_cli.commands.install.APMPackage") From 56688ff20becf064bc80c27795c785f171117ff4 Mon Sep 17 00:00:00 2001 From: Copilot Date: Sat, 2 May 2026 11:18:10 +0200 Subject: [PATCH 5/5] fix(validation): surface dep-only escape hatch in cascade-exit error (#1094) Adds a boundary integration test for the INVALID leaf of the validation cascade (apm.yml present, no .apm/, no declared deps). The test exposed a UX gap: the cascade-exit error message at validation.py:368-373 did not mention the dep-only escape hatch added in this PR, so users hitting the fence would not discover that declaring dependencies in apm.yml is now a valid alternative to .apm/. Updates the cascade-exit message to surface all three valid recovery paths (add .apm/, declare dependencies, add SKILL.md), keeping it consistent with the validator-level message at validation.py:747-752. Co-authored-by: xuyuanhao Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/models/validation.py | 3 +- tests/integration/test_apm_dependencies.py | 35 ++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/models/validation.py b/src/apm_cli/models/validation.py index 67e7214c6..1f89a00b2 100644 --- a/src/apm_cli/models/validation.py +++ b/src/apm_cli/models/validation.py @@ -368,7 +368,8 @@ def validate_apm_package(package_path: Path) -> ValidationResult: result.add_error( f"Not a valid APM package: {package_path.name} has apm.yml but " "is missing the required .apm/ directory. " - "Add .apm/ with primitives (instructions, skills, etc.) " + "Add .apm/ with primitives (instructions, skills, etc.), " + "declare dependencies in apm.yml (curated aggregator), " "or add skills//SKILL.md for a skill bundle." ) else: diff --git a/tests/integration/test_apm_dependencies.py b/tests/integration/test_apm_dependencies.py index fcadd2777..d4a8e9140 100644 --- a/tests/integration/test_apm_dependencies.py +++ b/tests/integration/test_apm_dependencies.py @@ -73,6 +73,41 @@ def create_apm_yml(self, dependencies: list[str] = None, **kwargs): # noqa: RUF return config + @pytest.mark.integration + def test_apm_yml_without_deps_or_apm_dir_rejects_as_invalid(self): + """Regression-trap for #1094 BOUNDARY: apm.yml with NO deps and NO + `.apm/` must be rejected with an actionable diagnostic, not + silently accepted as a valid dep-only aggregator. + + This is the negative fence of the dep-only rework. The cascade + classifies this shape as INVALID and the validator surfaces the + original "missing .apm/" guidance, now extended to mention the + dep-only and skill-bundle escape hatches. + + If a future refactor over-relaxes ``_apm_yml_declares_dependencies`` + (e.g., starts returning True for empty deps) the cascade would + silently classify garbage as APM_PACKAGE and the install pipeline + would no-op without an error -- this test catches that regression + through the same ``validate_apm_package`` entry point the install + pipeline uses. + """ + from apm_cli.models.validation import PackageType, validate_apm_package + + apm_yml = self.test_dir / "apm.yml" + apm_yml.write_text( + "name: empty-project\nversion: 1.0.0\ndescription: Project with no deps and no .apm/\n" + ) + assert not (self.test_dir / ".apm").exists(), "precondition: no .apm/ directory" + + result = validate_apm_package(self.test_dir) + assert result.is_valid is False + assert result.package_type == PackageType.INVALID + # Actionable diagnostic: must name the .apm/ requirement AND the + # dep-only escape hatch (so users discover the #1094 fix). + joined = " ".join(result.errors).lower() + assert ".apm" in joined + assert "declare dependencies" in joined # dep-only escape hatch surfaced + @pytest.mark.integration def test_dep_only_project_installs_dependencies_without_dot_apm(self): """Regression-trap for #1094: a dep-only `apm.yml` (no `.apm/` on the