From 5b2853539b22644426b6c47ea14fb6aa625a76aa Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Fri, 10 Jul 2026 12:01:25 +0100 Subject: [PATCH] Fix --skill filter silent no-op: emit warning when zero skills match When --skill is passed but no skill in the package matches the filter, _integrate_skill_bundle and _promote_sub_skills_standalone now emit _rich_warning listing the requested names and available skill names. Exit code stays 0; only the missing diagnostic is added. Mirrors the existing warning precedent for --skill on a native CLAUDE_SKILL (skill_integrator.py:1358). Closes #2116 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../.apm/skills/apm-usage/commands.md | 2 +- src/apm_cli/integration/skill_integrator.py | 28 ++++ .../test_skill_integrator_hermetic.py | 144 ++++++++++++++++++ 3 files changed, 173 insertions(+), 1 deletion(-) diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index cf98f2697..b70cba960 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -10,7 +10,7 @@ | Command | Purpose | Key flags | |---------|---------|-----------| -| `apm install [PKGS...]` | Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` (deprecated; prefer `apm update`) refresh refs, `--refresh` re-fetch all deps from upstream and re-resolve all ref pins, `--force` overwrite (does NOT refresh refs; use `apm update` for that), `--frozen` CI-safe install that fails fast when `apm.lock.yaml` is missing or out of sync with `apm.yml` (mutually exclusive with `--update`; structural presence check only -- use `apm audit` for SHA integrity), `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated, e.g. `--target claude,cursor`; highest-priority entry in the resolution chain `--target` > apm.yml `targets:` > auto-detect; on auto-bootstrap when no `apm.yml` exists, recognized manifest target(s) are persisted to the new manifest's `targets:` so a later bare `apm update` reuses them; `--target all` deprecated, see `apm compile --all`; use `kiro` for Kiro IDE; use `copilot-cowork` with `--global` after `apm experimental enable copilot-cowork`; use `hermes` after `apm experimental enable hermes` to deploy skills + `AGENTS.md` and, at `--global`, MCP servers to `~/.hermes/config.yaml`), `--dev`, `-g` global (MCP deploys only to user-scope runtimes: Copilot CLI, Claude Code, Codex CLI, Gemini CLI, Antigravity CLI, Kiro, Windsurf, JetBrains Copilot, and Hermes when enabled), `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--skill NAME` install named skill(s) from a skill collection (SKILL_BUNDLE or plugin manifest; repeatable; plugin manifests accept a leaf name or manifest path; persisted in apm.yml; additive across separate installs -- a later `--skill X` adds to the existing pin (union) rather than replacing it, so previously deployed skills are never silently removed; `'*'` resets to the full bundle; drop a single skill by editing the `skills:` list in apm.yml then re-running install), `--legacy-skill-paths` restore per-client skill dirs, `--mcp NAME` add MCP entry (NAME goes through the same `--target` > `targets:` > auto-detect resolver as APM packages, so a project whitelisting `targets: [copilot]` will not write `.cursor/mcp.json` even if `.cursor/` exists; `apm install -g --mcp NAME` writes user-scope and bypasses the project-scope gate by design), `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry, `--root DIR` redirect writes (`apm_modules/`, lockfile, `.gitignore`, integrated harness files) under DIR while `apm.yml`/`.apm/`/local deps resolve from `$PWD` (mirrors `pip install --target`; created if missing; not valid with `-g`/`--global`, which exits 2) | +| `apm install [PKGS...]` | Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` (deprecated; prefer `apm update`) refresh refs, `--refresh` re-fetch all deps from upstream and re-resolve all ref pins, `--force` overwrite (does NOT refresh refs; use `apm update` for that), `--frozen` CI-safe install that fails fast when `apm.lock.yaml` is missing or out of sync with `apm.yml` (mutually exclusive with `--update`; structural presence check only -- use `apm audit` for SHA integrity), `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated, e.g. `--target claude,cursor`; highest-priority entry in the resolution chain `--target` > apm.yml `targets:` > auto-detect; on auto-bootstrap when no `apm.yml` exists, recognized manifest target(s) are persisted to the new manifest's `targets:` so a later bare `apm update` reuses them; `--target all` deprecated, see `apm compile --all`; use `kiro` for Kiro IDE; use `copilot-cowork` with `--global` after `apm experimental enable copilot-cowork`; use `hermes` after `apm experimental enable hermes` to deploy skills + `AGENTS.md` and, at `--global`, MCP servers to `~/.hermes/config.yaml`), `--dev`, `-g` global (MCP deploys only to user-scope runtimes: Copilot CLI, Claude Code, Codex CLI, Gemini CLI, Antigravity CLI, Kiro, Windsurf, JetBrains Copilot, and Hermes when enabled), `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--skill NAME` install named skill(s) from a skill collection (SKILL_BUNDLE or plugin manifest; repeatable; plugin manifests accept a leaf name or manifest path; persisted in apm.yml; additive across separate installs -- a later `--skill X` adds to the existing pin (union) rather than replacing it, so previously deployed skills are never silently removed; `'*'` resets to the full bundle; drop a single skill by editing the `skills:` list in apm.yml then re-running install; when the filter matches zero available skills a warning is printed listing what was requested and what is available), `--legacy-skill-paths` restore per-client skill dirs, `--mcp NAME` add MCP entry (NAME goes through the same `--target` > `targets:` > auto-detect resolver as APM packages, so a project whitelisting `targets: [copilot]` will not write `.cursor/mcp.json` even if `.cursor/` exists; `apm install -g --mcp NAME` writes user-scope and bypasses the project-scope gate by design), `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry, `--root DIR` redirect writes (`apm_modules/`, lockfile, `.gitignore`, integrated harness files) under DIR while `apm.yml`/`.apm/`/local deps resolve from `$PWD` (mirrors `pip install --target`; created if missing; not valid with `-g`/`--global`, which exits 2) | | `apm targets` | Show resolved deployment targets for the current project (Click group; reads filesystem signals; works with or without `apm.yml`) | `--all` also include the `agent-skills` meta-target (only meaningful with `--json`), `--json` machine-readable output. No provenance line is printed (the table is the provenance). | | `apm uninstall PKGS...` | Remove packages (accepts `owner/repo` or `name@marketplace`) | `--dry-run`, `-g` global | | `apm prune` | Remove orphaned packages | `--dry-run` | diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index fc59b03d0..659b3e641 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -893,6 +893,20 @@ def _promote_sub_skills_standalone( count = n all_deployed.extend(deployed) + if name_filter is not None and count == 0: + from apm_cli.utils.console import _rich_warning + + available = sorted( + d.name for d in sub_skills_dir.iterdir() if d.is_dir() and (d / "SKILL.md").exists() + ) + available_str = ", ".join(available) if available else "(none)" + requested_str = ", ".join(sorted(name_filter)) + _rich_warning( + f"--skill filter matched no skills in this package. " + f"Requested: {requested_str}. " + f"Available: {available_str}." + ) + return count, all_deployed def _integrate_native_skill( @@ -1232,6 +1246,20 @@ def _integrate_skill_bundle( any_created = True all_deployed.extend(deployed) + if _name_filter is not None and total_promoted == 0: + from apm_cli.utils.console import _rich_warning + + available = sorted( + d.name for d in skills_dir.iterdir() if d.is_dir() and (d / "SKILL.md").exists() + ) + available_str = ", ".join(available) if available else "(none)" + requested_str = ", ".join(sorted(_name_filter)) + _rich_warning( + f"--skill filter matched no skills in this package. " + f"Requested: {requested_str}. " + f"Available: {available_str}." + ) + return SkillIntegrationResult( skill_created=any_created, skill_updated=False, diff --git a/tests/unit/integration/test_skill_integrator_hermetic.py b/tests/unit/integration/test_skill_integrator_hermetic.py index 86554d18e..6f10bebfb 100644 --- a/tests/unit/integration/test_skill_integrator_hermetic.py +++ b/tests/unit/integration/test_skill_integrator_hermetic.py @@ -950,3 +950,147 @@ def test_promote_sub_skills_includes_bin_when_approved(self, tmp_path: Path) -> assert (promoted_skill / "bin" / "helper").exists(), ( "bin/ must be included when skip_bin=False" ) + + +# --------------------------------------------------------------------------- +# _integrate_skill_bundle -- --skill filter matches nothing (issue #2116) +# --------------------------------------------------------------------------- + + +class TestIntegrateSkillBundleSubsetNoMatch: + """--skill filter that matches zero skills emits a warning with available names.""" + + def test_no_match_warns_with_available_names(self, tmp_path: Path) -> None: + pkg_dir = tmp_path / "bundle-pkg" + pkg_dir.mkdir() + skills_dir = pkg_dir / "skills" + tdd_dir = skills_dir / "tdd" + tdd_dir.mkdir(parents=True) + (tdd_dir / "SKILL.md").write_text("# tdd skill", encoding="utf-8") + + dep_ref = MagicMock() + dep_ref.is_virtual = False + dep_ref.get_unique_key.return_value = "owner/bundle-pkg" + pi = _make_package_info(pkg_dir, dep_ref=dep_ref) + + target = _make_target(name="copilot", root_dir=".github") + integrator = SkillIntegrator() + + with ( + patch("apm_cli.utils.console._rich_warning") as mock_warn, + patch.object(SkillIntegrator, "_build_ownership_maps", return_value=({}, {})), + ): + integrator._integrate_skill_bundle( + pi, + tmp_path, + skills_dir, + targets=[target], + skill_subset=("nonexistent",), + ) + + mock_warn.assert_called_once() + warning_msg = mock_warn.call_args[0][0] + assert "nonexistent" in warning_msg + assert "tdd" in warning_msg + + def test_no_match_does_not_warn_when_no_filter(self, tmp_path: Path) -> None: + """No warning when skill_subset is None (all skills are deployed).""" + pkg_dir = tmp_path / "bundle-pkg" + pkg_dir.mkdir() + skills_dir = pkg_dir / "skills" + tdd_dir = skills_dir / "tdd" + tdd_dir.mkdir(parents=True) + (tdd_dir / "SKILL.md").write_text("# tdd skill", encoding="utf-8") + + dep_ref = MagicMock() + dep_ref.is_virtual = False + dep_ref.get_unique_key.return_value = "owner/bundle-pkg" + pi = _make_package_info(pkg_dir, dep_ref=dep_ref) + + target = _make_target(name="copilot", root_dir=".github") + integrator = SkillIntegrator() + + with ( + patch("apm_cli.utils.console._rich_warning") as mock_warn, + patch.object(SkillIntegrator, "_build_ownership_maps", return_value=({}, {})), + ): + integrator._integrate_skill_bundle( + pi, + tmp_path, + skills_dir, + targets=[target], + skill_subset=None, + ) + + mock_warn.assert_not_called() + + +# --------------------------------------------------------------------------- +# _promote_sub_skills_standalone -- --skill filter matches nothing (#2116) +# --------------------------------------------------------------------------- + + +class TestPromoteSubSkillsStandaloneSubsetNoMatch: + """--skill filter that matches zero .apm/skills/ entries emits a warning.""" + + def test_no_match_warns_with_available_names(self, tmp_path: Path) -> None: + pkg_dir = tmp_path / "instr-pkg" + pkg_dir.mkdir() + sub_skills_dir = pkg_dir / ".apm" / "skills" + tdd_dir = sub_skills_dir / "tdd" + tdd_dir.mkdir(parents=True) + (tdd_dir / "SKILL.md").write_text("# tdd skill", encoding="utf-8") + + dep_ref = MagicMock() + dep_ref.is_virtual = False + dep_ref.get_unique_key.return_value = "owner/instr-pkg" + pi = _make_package_info(pkg_dir, dep_ref=dep_ref) + + target = _make_target(name="copilot", root_dir=".github") + integrator = SkillIntegrator() + + with ( + patch("apm_cli.utils.console._rich_warning") as mock_warn, + patch.object(SkillIntegrator, "_build_skill_ownership_map", return_value={}), + ): + integrator._promote_sub_skills_standalone( + pi, + tmp_path, + targets=[target], + skill_subset=("nonexistent",), + ) + + mock_warn.assert_called_once() + warning_msg = mock_warn.call_args[0][0] + assert "nonexistent" in warning_msg + assert "tdd" in warning_msg + + def test_no_match_does_not_warn_when_no_filter(self, tmp_path: Path) -> None: + """No warning when skill_subset is None.""" + pkg_dir = tmp_path / "instr-pkg" + pkg_dir.mkdir() + sub_skills_dir = pkg_dir / ".apm" / "skills" + tdd_dir = sub_skills_dir / "tdd" + tdd_dir.mkdir(parents=True) + (tdd_dir / "SKILL.md").write_text("# tdd skill", encoding="utf-8") + + dep_ref = MagicMock() + dep_ref.is_virtual = False + dep_ref.get_unique_key.return_value = "owner/instr-pkg" + pi = _make_package_info(pkg_dir, dep_ref=dep_ref) + + target = _make_target(name="copilot", root_dir=".github") + integrator = SkillIntegrator() + + with ( + patch("apm_cli.utils.console._rich_warning") as mock_warn, + patch.object(SkillIntegrator, "_build_skill_ownership_map", return_value={}), + ): + integrator._promote_sub_skills_standalone( + pi, + tmp_path, + targets=[target], + skill_subset=None, + ) + + mock_warn.assert_not_called()