From 444636e071c2ea624ba898ac0231fdc73169cb5f Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 4 Sep 2026 15:05:05 -0400 Subject: [PATCH 1/4] fix(install): fail target-excluded plugin no-ops Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 6 + src/apm_cli/install/outcome.py | 16 ++ src/apm_cli/install/template.py | 80 +++++++- src/apm_cli/utils/diagnostics.py | 35 +++- .../test_agent_plugin_deployment_boundary.py | 177 ++++++++++++------ 5 files changed, 253 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7810ac4b1..4192e808f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- BREAKING: `apm install` now exits non-zero when target exclusion leaves an + Agent Plugins v1.0.0 package deployed to no target. Migration: install an + individual skill subpath such as + `apm install //skills/ --target `, or select + `--target copilot` for native registration; mixed installs still exit 0 when + at least one package deploys. (closes #2796) (#2806) - Hermes is now a stable explicit-only target, and state-mutating APM commands share one per-user cross-process lock to prevent concurrent updates from losing state. (by @lkshrk; closes #2608) (#2655) diff --git a/src/apm_cli/install/outcome.py b/src/apm_cli/install/outcome.py index de1ba6cc92..d8133dac67 100644 --- a/src/apm_cli/install/outcome.py +++ b/src/apm_cli/install/outcome.py @@ -41,6 +41,21 @@ def diagnostic_error_count(diagnostics: object | None) -> int: return 0 +def agent_plugin_target_excluded_count(diagnostics: object | None) -> int: + """Return the number of target-excluded Agent Plugin packages.""" + if diagnostics is None: + return 0 + value = getattr(diagnostics, "agent_plugin_target_excluded_count", 0) + if isinstance(value, int): + return value + if not isinstance(value, str): + return 0 + try: + return int(value) + except (TypeError, ValueError): + return 0 + + def install_command_outcome(result: InstallResult) -> InstallCommandOutcome: """Return the command outcome owned by ``InstallResult.disposition``.""" if result.disposition in _SUCCESS_SUMMARY_DISPOSITIONS: @@ -143,6 +158,7 @@ def finalize_install_result( if ( result.disposition is InstallDisposition.FAILED or diagnostic_error_count(diagnostics) > 0 + or (result.installed_count == 0 and agent_plugin_target_excluded_count(diagnostics) > 0) or (has_critical and not force) ): result.disposition = InstallDisposition.FAILED diff --git a/src/apm_cli/install/template.py b/src/apm_cli/install/template.py index d5c265292d..23ab863888 100644 --- a/src/apm_cli/install/template.py +++ b/src/apm_cli/install/template.py @@ -242,17 +242,86 @@ def _record_agent_plugin_boundary_failure( return deltas +def _target_names_for_hint(ctx: InstallContext) -> str: + """Return the target argument to use in a recovery command.""" + names: list[str] = [] + for target in getattr(ctx, "targets", None) or (): + name = getattr(target, "name", target) + if isinstance(name, str) and name: + names.append(name) + return ",".join(sorted(set(names))) or "" + + +def _dependency_base_for_skill_subpath(dep_ref) -> str: + """Return the dependency reference before any ref suffix.""" + if getattr(dep_ref, "is_local", False) and getattr(dep_ref, "local_path", None): + display = dep_ref.local_path + elif hasattr(dep_ref, "to_display_reference"): + display = dep_ref.to_display_reference() + elif hasattr(dep_ref, "get_identity"): + display = dep_ref.get_identity() + else: + display = getattr(dep_ref, "repo_url", "") + return str(display).split("#", 1)[0].rstrip("/") + + +def _agent_plugin_skill_name_for_hint( + source: DependencySource, + materialization: Materialization, +) -> str | None: + """Return one skill name that can be installed through the subpath route.""" + requested = getattr(source.ctx, "skill_subset", None) + if requested: + for value in requested: + skill_name = str(value) + if skill_name and skill_name != "*": + return skill_name + + package = getattr(materialization.package_info, "package", None) + plugin = getattr(package, "agent_plugin", None) + components = getattr(plugin, "components", None) + for skill in getattr(components, "skills", ()) or (): + skill_name = getattr(skill, "directory_name", None) or getattr(skill, "name", None) + if skill_name: + return str(skill_name) + return None + + +def _agent_plugin_target_skip_message( + source: DependencySource, + materialization: Materialization, + error: AgentPluginTargetExcludedError, +) -> str: + """Return the actionable target-exclusion diagnostic for one package.""" + message = str(error) + skill_name = _agent_plugin_skill_name_for_hint(source, materialization) + if skill_name is None: + return ( + f"{message} No selected target received this package. " + "Select --target copilot or install a target-compatible package." + ) + + dep_base = _dependency_base_for_skill_subpath(source.dep_ref) + target_arg = _target_names_for_hint(source.ctx) + command = f"apm install {dep_base}/skills/{skill_name} --target {target_arg}" + return ( + f"{message} No selected target received this package. " + "To install this skill as a plain skill bundle, use " + f"'{command}'." + ) + + def _record_agent_plugin_target_skip( source: DependencySource, materialization: Materialization, error: AgentPluginTargetExcludedError, ) -> dict[str, int]: - """Record a non-fatal skip for a package this project does not target at copilot. + """Record a target skip for a package this project does not target at copilot. Mirrors the per-dependency ``targets:`` subset already handled in ``finalize_native_plugin``: native registration is skipped, ONE warning - names the package, and the rest of the batch installs. This is a warning, - not an error, so the install still exits 0. + names the package, and the rest of the batch can still install. The + install outcome owner fails later only when no package deployed. """ ctx = source.ctx deltas = materialization.deltas @@ -261,7 +330,10 @@ def _record_agent_plugin_target_skip( deltas["installed"] = 0 ctx.package_deployed_files[dep_key] = [] package_key = dep_ref.local_path if (dep_ref.is_local and dep_ref.local_path) else dep_key - ctx.diagnostics.warn(str(error), package=package_key) + ctx.diagnostics.agent_plugin_target_excluded( + _agent_plugin_target_skip_message(source, materialization, error), + package=package_key, + ) return deltas diff --git a/src/apm_cli/utils/diagnostics.py b/src/apm_cli/utils/diagnostics.py index 6ede707b2f..34c634668f 100644 --- a/src/apm_cli/utils/diagnostics.py +++ b/src/apm_cli/utils/diagnostics.py @@ -21,6 +21,7 @@ CATEGORY_COLLISION = "collision" CATEGORY_OVERWRITE = "overwrite" CATEGORY_WARNING = "warning" +WARNING_AGENT_PLUGIN_TARGET_EXCLUDED = "agent_plugin_target_excluded" # Reserved for agent source semantics dropped during target-format translation. CATEGORY_AGENT_LOSSY_COMPILATION = "agent_lossy_compilation" CATEGORY_ERROR = "error" @@ -63,7 +64,7 @@ class Diagnostic: category: str package: str = "" detail: str = "" - severity: str = "" # e.g. "critical", "warning", "info" -- used by security category + severity: str = "" # e.g. "critical", "warning", "info", or warning subtype class DiagnosticCollector: @@ -106,7 +107,13 @@ def overwrite(self, path: str, package: str = "", detail: str = "") -> None: ) ) - def warn(self, message: str, package: str = "", detail: str = "") -> None: + def warn( + self, + message: str, + package: str = "", + detail: str = "", + severity: str = "", + ) -> None: """Record a general warning.""" with self._lock: self._diagnostics.append( @@ -115,9 +122,24 @@ def warn(self, message: str, package: str = "", detail: str = "") -> None: category=CATEGORY_WARNING, package=package, detail=detail, + severity=severity, ) ) + def agent_plugin_target_excluded( + self, + message: str, + package: str = "", + detail: str = "", + ) -> None: + """Record an Agent Plugin target exclusion warning.""" + self.warn( + message, + package=package, + detail=detail, + severity=WARNING_AGENT_PLUGIN_TARGET_EXCLUDED, + ) + def lossy_agent_compilation( self, message: str, @@ -253,6 +275,15 @@ def has_diagnostics(self) -> bool: def error_count(self) -> int: return sum(1 for d in self._diagnostics if d.category == CATEGORY_ERROR) + @property + def agent_plugin_target_excluded_count(self) -> int: + """Return target-excluded Agent Plugin warning count.""" + return sum( + 1 + for d in self._diagnostics + if d.category == CATEGORY_WARNING and d.severity == WARNING_AGENT_PLUGIN_TARGET_EXCLUDED + ) + @property def security_count(self) -> int: """Return number of security findings.""" diff --git a/tests/unit/install/test_agent_plugin_deployment_boundary.py b/tests/unit/install/test_agent_plugin_deployment_boundary.py index 7eba212127..4743559a07 100644 --- a/tests/unit/install/test_agent_plugin_deployment_boundary.py +++ b/tests/unit/install/test_agent_plugin_deployment_boundary.py @@ -31,7 +31,10 @@ integrate_package_primitives, ) from apm_cli.install.sources import Materialization -from apm_cli.install.template import run_integration_template +from apm_cli.install.template import ( + _agent_plugin_target_skip_message, + run_integration_template, +) from apm_cli.integration.hook_integrator import HookIntegrator from apm_cli.integration.skill_integrator import SkillIntegrator, get_effective_type from apm_cli.models.apm_package import APMPackage, PackageContentType, PackageInfo @@ -293,7 +296,7 @@ def acquire(self) -> Materialization: ("registry", False, "registry", 1, "Failed to integrate primitives from cached package"), ], ) -def test_every_materialization_shape_skips_without_committing_state_when_target_excludes_copilot( +def test_every_materialization_shape_fails_when_target_exclusion_deploys_nothing( tmp_path: Path, shape: str, is_local: bool, @@ -301,15 +304,15 @@ def test_every_materialization_shape_skips_without_committing_state_when_target_ initial_installed: int, error_prefix: str, ) -> None: - """A non-Copilot target set is a routine, non-fatal skip for every shape. + """A non-Copilot target set skips safely for every materialization shape. ``ctx.targets`` here is a mock target that never resolves to ``copilot``, so the capability owner reports ``AgentPluginTargetExcludedError`` -- never a fatal boundary error, and never dependent on whether a Copilot binary exists on this host. This holds across every materialization shape (local/cached/fresh/registry): the package is skipped with one - warning, no integrator ever mutates the project tree, and the batch - still exits 0. + warning and no integrator ever mutates the project tree. Issue #2796 then + makes the install outcome fail because no package was deployed. """ package_info = _write_adversarial_agent_plugin( tmp_path / f"{shape}-source", @@ -362,13 +365,14 @@ def test_every_materialization_shape_skips_without_committing_state_when_target_ assert deltas["installed"] == 0 assert ctx.package_deployed_files == {f"blocked/{shape}": []} assert diagnostics.error_count == 0 + assert diagnostics.agent_plugin_target_excluded_count == 1 assert len(diagnostics._diagnostics) == 1 assert diagnostics._diagnostics[0].category == "warning" result = finalize_install_result( InstallResult(installed_count=deltas["installed"], diagnostics=diagnostics), force=True, ) - assert result.exit_code == 0 + assert result.exit_code == 1 assert _tree_snapshot(project) == before for integrator in integrator_map.values(): assert integrator.mock_calls == [] @@ -440,19 +444,19 @@ def _write_ordinary_package(root: Path) -> None: ("--skill", "safe"), ), ) -def test_target_exclusion_skips_the_plugin_without_touching_the_rest_of_the_batch( +def test_target_exclusion_does_not_hard_fail_when_the_rest_of_the_batch_installs( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, native_first: bool, extra_args: tuple[str, ...], ) -> None: - """A batch with an otherwise-valid native plugin excluded by target. + """A mixed batch with an otherwise-valid native plugin excluded by target. Admission is a pure function of resolved target names, so excluding - ``copilot`` is the ONLY way a canonical Agent Plugin is refused -- and it - is always non-fatal (Item 4): the plugin is skipped with one warning and - the rest of the batch installs, regardless of dependency ordering or - which install flags are in play. + ``copilot`` is the ONLY way a canonical Agent Plugin is refused. Issue + #2796 makes a pure no-op fail, but this mixed install must stay non-fatal: + the plugin is skipped with one warning and the rest of the batch installs, + regardless of dependency ordering or which install flags are in play. """ workspace = tmp_path / "workspace" project = workspace / "project" @@ -498,6 +502,8 @@ def tracked_available(package_info): catch_exceptions=False, ) + # Issue #2796: exit 0 is intentional only because the ordinary package was + # deployed; a target-excluded Agent Plugin alone must fail loudly. assert result.exit_code == 0, result.output # The excluded plugin is never handed to any integrator -- native or # legacy -- it is dropped outright, not decomposed. @@ -515,22 +521,90 @@ def tracked_available(package_info): assert "ask the publisher for a legacy-compatible package" not in output +def test_target_exclusion_fails_when_no_package_is_deployed( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Issue #2796: a target-excluded Agent Plugin cannot be a successful no-op.""" + project = tmp_path / "project" + native = tmp_path / "native" + project.mkdir() + _write_adversarial_agent_plugin(native, tmp_path / "outside.txt") + (native / "skills" / "native" / "nested" / "outside-link").unlink() + (project / "apm.yml").write_text( + json.dumps( + { + "name": "consumer", + "version": "1.0.0", + "dependencies": {"apm": ["../native"]}, + "target": "codex", + } + ), + encoding="ascii", + ) + native_integrator_calls: list[str] = [] + original_integrate = SkillIntegrator.integrate_package_skill + + def tracked_integrate(self, package_info, *args, **kwargs): + if package_info.package_type is PackageType.AGENT_PLUGIN: + native_integrator_calls.append("integrate") + return original_integrate(self, package_info, *args, **kwargs) + + monkeypatch.setattr(SkillIntegrator, "integrate_package_skill", tracked_integrate) + monkeypatch.chdir(project) + + result = CliRunner().invoke( + cli, + ["install", "--no-policy", "--target", "codex", "--skill", "native"], + catch_exceptions=False, + ) + + assert result.exit_code != 0, result.output + assert native_integrator_calls == [] + output = " ".join(result.output.split()) + assert "No selected target received this package" in output + assert "apm install ../native/skills/native --target codex" in output + assert not (project / ".codex" / "skills" / "native").exists() + assert not (project / ".agents" / "skills" / "native").exists() + + +def test_target_exclusion_hint_names_remote_skill_subpath_form() -> None: + """The no-op error must name the working subpath workaround from #2796.""" + source = SimpleNamespace( + ctx=SimpleNamespace( + targets=[SimpleNamespace(name="codex")], + skill_subset=("lavish",), + ), + dep_ref=SimpleNamespace( + is_local=False, + to_display_reference=lambda: "kunchenguid/lavish-axi#main", + ), + ) + materialization = SimpleNamespace(package_info=SimpleNamespace(package=None)) + + message = _agent_plugin_target_skip_message( + source, + materialization, + AgentPluginTargetExcludedError("target excluded"), + ) + + assert "apm install kunchenguid/lavish-axi/skills/lavish --target codex" in message + assert "#main/skills" not in message + + @pytest.mark.parametrize("include_ordinary", (False, True)) -def test_dry_run_target_exclusion_is_non_fatal_like_a_real_install( +def test_dry_run_target_exclusion_is_non_fatal_preview( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, include_ordinary: bool, ) -> None: - """``--dry-run`` must not fatal-fail on an outcome a real install tolerates. - - Admission is a pure function of resolved target names. A real install - skips an excluded native plugin with one non-fatal warning and installs - the rest of the batch (Item 4); the dry-run preflight - (``preflight_agent_plugin_dry_run``) evaluates the SAME - ``enforce_agent_plugin_deployment_boundary`` call and must swallow the - same ``AgentPluginTargetExcludedError`` instead of aborting the whole - preview -- regression coverage for a real dry-run/real-install exit-code - parity bug this refactor uncovered and fixed. + """``--dry-run`` must not fatal-fail while previewing target exclusion. + + Admission is a pure function of resolved target names. Issue #2796 makes a + real no-op fail, but dry-run still previews without mutating targets. The + preflight evaluates the SAME ``enforce_agent_plugin_deployment_boundary`` + call and must swallow the same ``AgentPluginTargetExcludedError`` instead + of aborting the preview. """ workspace = tmp_path / "workspace" native_sources = [workspace / "native-a", workspace / "native-b"] @@ -571,42 +645,35 @@ def tracked_available(package_info): source: _tree_snapshot(source) for source in [*native_sources, *([ordinary] if include_ordinary else [])] } - outputs = {} - for mode, extra_args in (("real", ()), ("dry-run", ("--dry-run",))): - project = workspace / mode - project.mkdir() - (project / "apm.yml").write_text( - json.dumps( - { - "name": f"consumer-{mode}", - "version": "1.0.0", - "dependencies": {"apm": dependencies}, - "target": "claude", - } - ), - encoding="ascii", - ) - monkeypatch.chdir(project) + project = workspace / "dry-run" + project.mkdir() + (project / "apm.yml").write_text( + json.dumps( + { + "name": "consumer-dry-run", + "version": "1.0.0", + "dependencies": {"apm": dependencies}, + "target": "claude", + } + ), + encoding="ascii", + ) + monkeypatch.chdir(project) - result = CliRunner().invoke( - cli, - ["install", "--no-policy", "--target", "claude", *extra_args], - catch_exceptions=False, - ) + result = CliRunner().invoke( + cli, + ["install", "--dry-run", "--no-policy", "--target", "claude"], + catch_exceptions=False, + ) - # The exit-code parity is the whole point of this regression: before - # the fix, dry-run raised AgentPluginTargetExcludedError uncaught - # (exit 1) for the exact scenario a real install tolerates (exit 0). - assert result.exit_code == 0, result.output - for source, snapshot in source_snapshots.items(): - assert _tree_snapshot(source) == snapshot - outputs[mode] = " ".join(result.output.split()) + assert result.exit_code == 0, result.output + for source, snapshot in source_snapshots.items(): + assert _tree_snapshot(source) == snapshot + output = " ".join(result.output.split()) assert native_integrator_calls == [] - # Only the real install renders the per-package skip warning; the - # dry-run preflight is reject-only and does not render diagnostics. - assert outputs["real"].count("Re-run with --target copilot") == 2 - assert outputs["dry-run"].count("Re-run with --target copilot") == 0 + # The dry-run preflight is reject-only and does not render diagnostics. + assert output.count("Re-run with --target copilot") == 0 def test_dry_run_target_exclusion_uses_the_explicit_target( From 4cb7bc052eabe8c34049216bfbf8d444621f0993 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 4 Sep 2026 15:22:44 -0400 Subject: [PATCH 2/4] fix: preserve agent plugin recovery refs Addresses panel follow-ups to keep Agent Plugin target-exclusion recovery commands pinned to the selected ref, document the breaking no-op contract, and add real CLI lifecycle coverage for total no-op failure plus mixed-install success. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 7 +- .../src/content/docs/reference/cli/install.md | 5 +- .../content/docs/reference/package-types.md | 14 ++- .../.apm/skills/apm-usage/commands.md | 6 + src/apm_cli/install/template.py | 29 +++-- .../test_required_lifecycle_state_machine.py | 119 ++++++++++++++++++ .../test_agent_plugin_deployment_boundary.py | 2 +- 7 files changed, 160 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4192e808f5..cb9f4cd083 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - BREAKING: `apm install` now exits non-zero when target exclusion leaves an Agent Plugins v1.0.0 package deployed to no target. Migration: install an individual skill subpath such as - `apm install //skills/ --target `, or select - `--target copilot` for native registration; mixed installs still exit 0 when - at least one package deploys. (closes #2796) (#2806) + `apm install //skills/ --target ` (for example, + `apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`), or + select `--target copilot` for native registration; mixed installs still exit + 0 when at least one package deploys. (closes #2796) (#2806) - Hermes is now a stable explicit-only target, and state-mutating APM commands share one per-user cross-process lock to prevent concurrent updates from losing state. (by @lkshrk; closes #2608) (#2655) diff --git a/docs/src/content/docs/reference/cli/install.md b/docs/src/content/docs/reference/cli/install.md index 07607543f1..bb646c39c3 100644 --- a/docs/src/content/docs/reference/cli/install.md +++ b/docs/src/content/docs/reference/cli/install.md @@ -266,13 +266,14 @@ apm install owner/skill-bundle --skill '*' # reset to all skills | Code | Meaning | |---|---| -| `0` | Success. All requested dependencies and local content deployed. | -| `1` | Install failure: security scan blocked a critical finding, auth error, manifest or required MCP/LSP config write error, dependency resolution error, `--frozen` with a missing lockfile or a direct dependency absent from `apm.lock.yaml`, any reported install error (the diagnostic summary closes with `Installation failed with N error(s)`), or unhandled exception. `--force` does **not** suppress general install errors. The diagnostic summary names the cause. | +| `0` | Success. All requested dependencies and local content deployed. For Agent Plugins v1 packages, a mixed install still succeeds when target exclusion skips one package but at least one other package deploys. | +| `1` | Install failure: security scan blocked a critical finding, auth error, manifest or required MCP/LSP config write error, dependency resolution error, Agent Plugins v1 target exclusion left no package deployed, `--frozen` with a missing lockfile or a direct dependency absent from `apm.lock.yaml`, any reported install error (the diagnostic summary closes with `Installation failed with N error(s)`), or unhandled exception. `--force` does **not** suppress general install errors. The diagnostic summary names the cause. | | `2` | Usage error: no deployment target detectable (no `--target`, no `target(s):` in `apm.yml`, no default target configured via `apm config set target `, and no harness signal in the project), `--ssh` and `--https` both passed, `--frozen` and `--update` both passed, `--root` combined with `--global`, or a Click flag conflict. | ## Notes - **`--force` is dual-purpose.** It overwrites locally-authored files on collision **and** disables the critical-finding block from the built-in security scan. It does **not** suppress general install errors -- any error reported in the diagnostic summary still exits `1` (matches `npm` / `pip` / `cargo`). It does **not** refresh remote refs -- for routine ref updates, run [`apm update`](../update/). To remediate a blocked package, fix the reported source files and reinstall; `apm audit --strip` only remediates files that are already deployed. See [Drift and secure by default](../../../consumer/drift-and-secure-by-default/). +- **Agent Plugin target exclusion is fail-loud on total no-ops.** Agent Plugins v1 packages register natively only with the effective `copilot` target today. If every selected target excludes native registration and no other package deploys, install exits `1` and prints a skill-subpath recovery command such as `apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`, or you can select `--target copilot` for native registration. Mixed installs that deploy at least one other package still exit `0`. - **Target contraction is reconciled.** A narrowed `targets:` in `apm.yml` is reconciled on the next non-dry-run install: deployed files, lockfile ownership, and merge-hook config/sidecar entries for the dropped target are cleaned up, even when no dependency itself changed. A package's own `target:` / `targets:` declaration applies an additional restriction within that effective set. See [Hooks and commands](../../../producer/author-primitives/hooks-and-commands/#hooks) for the full intersection and merge-hook config/sidecar details. `apm lock` may refresh the lockfile rows, but it never deletes deployed files from disk. - **Claude target prompt rewrite.** When deploying to `.claude/commands/`, prompt files with an `input:` front-matter key are rewritten to Claude's `arguments:` shape and `${input:name}` placeholders become `$name`. Argument names must match `^[A-Za-z][\w-]{0,63}$`; rejected names are dropped with a warning. - **MCP env-var passthrough.** Copilot CLI and Kiro translate `${env:VAR}` and `` to `${VAR}` in their MCP configs. Kiro writes `.kiro/settings/mcp.json` and `~/.kiro/settings/mcp.json` with `0o600` permissions. JetBrains Copilot preserves env references as `${env:VAR}` in `github-copilot/intellij/mcp.json`. Plaintext secrets are never written to disk for these runtime-resolved targets; legacy targets resolve placeholders at install time. diff --git a/docs/src/content/docs/reference/package-types.md b/docs/src/content/docs/reference/package-types.md index 03bb004c3b..44896e38c0 100644 --- a/docs/src/content/docs/reference/package-types.md +++ b/docs/src/content/docs/reference/package-types.md @@ -294,9 +294,17 @@ Any target set that excludes Copilot refuses this native install rather than falling back to the Claude plugin artifact mapping above -- APM never partially dissects a recognized Agent Plugin through its normal primitive integrators. Older Copilot clients may copy plugins into private state outside APM ownership, -so APM cannot guarantee cleanup of those client-created copies. Ask the -publisher for a Claude-compatible package (`apm pack --claude-plugin`) for a -non-Copilot target. +so APM cannot guarantee cleanup of those client-created copies. + +When target exclusion leaves a full Agent Plugins v1 package deployed to no +target, `apm install` exits `1` with a recovery command. To install one skill as +a plain skill bundle for another target, use +`apm install //skills/ --target `; refs stay after +the skill path, for example +`apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`. A mixed +install that deploys at least one other package still exits `0`. Publishers can +also provide a Claude-compatible package (`apm pack --claude-plugin`) for +non-Copilot targets. ::: **When to choose:** you are producing a portable package with diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index aa5062c973..ffd4585a80 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -141,6 +141,12 @@ writes only `.vscode/mcp.json`. `apm compile` continues to use legacy auto-detection with a `vscode`/`minimal` fallback for unsignalled projects -- bringing it onto the strict resolution chain is tracked as a follow-up. +Agent Plugins v1 packages register natively only with the effective `copilot` +target today. If target exclusion leaves an install with no deployed package, +`apm install` exits `1` and prints a recovery command such as +`apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`; mixed +installs that deploy at least one other package still exit `0`. + ## Compilation | Command | Purpose | Key flags | diff --git a/src/apm_cli/install/template.py b/src/apm_cli/install/template.py index 23ab863888..5466febee0 100644 --- a/src/apm_cli/install/template.py +++ b/src/apm_cli/install/template.py @@ -118,9 +118,10 @@ def preflight_agent_plugin_materializations( """Reject the batch once, before any package can mutate a target. A native Agent Plugin whose effective targets simply do not select - ``copilot`` is not a failure: it is skipped per-package during integration - (:func:`_record_agent_plugin_target_skip`), so it must not abort the - batch -- ``AgentPluginTargetExcludedError`` carries that distinction. + ``copilot`` is not a structural package failure: it is skipped per-package + during integration (:func:`_record_agent_plugin_target_skip`), so it must + not abort a batch that can still deploy other packages. The final install + outcome may still fail a pure no-op run. Anything else raised here (missing canonical IR, the imperative bundle route) is a real, actionable failure and aborts the whole batch. """ @@ -145,11 +146,12 @@ def preflight_agent_plugin_dry_run( 'no native harness' fallback. Admission never depends on whether a Copilot binary exists or which version it reports. - Target exclusion (``AgentPluginTargetExcludedError``) is never fatal -- - a real install skips that package with one warning and installs the - rest of the batch, so the dry-run preview must not abort the whole - preview for the same reason either. Only a genuine structural failure - (missing canonical IR, the imperative bundle route) aborts here. + Target exclusion (``AgentPluginTargetExcludedError``) is non-fatal during + preflight and per-package integration: a real install skips that package + with one warning and can still install the rest of the batch. The command + outcome owner may still fail a pure no-op install after integration. Only a + genuine structural failure (missing canonical IR, the imperative bundle + route) aborts here. """ from apm_cli.bundle.local_bundle import route_agent_plugin_package from apm_cli.copilot_plugins.capability import native_registration_scope @@ -252,8 +254,8 @@ def _target_names_for_hint(ctx: InstallContext) -> str: return ",".join(sorted(set(names))) or "" -def _dependency_base_for_skill_subpath(dep_ref) -> str: - """Return the dependency reference before any ref suffix.""" +def _dependency_parts_for_skill_subpath(dep_ref) -> tuple[str, str]: + """Return dependency base and ref suffix for a skill-subpath command.""" if getattr(dep_ref, "is_local", False) and getattr(dep_ref, "local_path", None): display = dep_ref.local_path elif hasattr(dep_ref, "to_display_reference"): @@ -262,7 +264,8 @@ def _dependency_base_for_skill_subpath(dep_ref) -> str: display = dep_ref.get_identity() else: display = getattr(dep_ref, "repo_url", "") - return str(display).split("#", 1)[0].rstrip("/") + base, separator, reference = str(display).partition("#") + return base.rstrip("/"), f"{separator}{reference}" if separator else "" def _agent_plugin_skill_name_for_hint( @@ -301,9 +304,9 @@ def _agent_plugin_target_skip_message( "Select --target copilot or install a target-compatible package." ) - dep_base = _dependency_base_for_skill_subpath(source.dep_ref) + dep_base, ref_suffix = _dependency_parts_for_skill_subpath(source.dep_ref) target_arg = _target_names_for_hint(source.ctx) - command = f"apm install {dep_base}/skills/{skill_name} --target {target_arg}" + command = f"apm install {dep_base}/skills/{skill_name}{ref_suffix} --target {target_arg}" return ( f"{message} No selected target received this package. " "To install this skill as a plain skill bundle, use " diff --git a/tests/integration/test_required_lifecycle_state_machine.py b/tests/integration/test_required_lifecycle_state_machine.py index c588373be1..6ff1694041 100644 --- a/tests/integration/test_required_lifecycle_state_machine.py +++ b/tests/integration/test_required_lifecycle_state_machine.py @@ -12,6 +12,7 @@ import pytest +from apm_cli.agent_plugins import PLUGIN_SCHEMA_ID from apm_cli.deps.lockfile import LockFile from apm_cli.integration.targets import KNOWN_TARGETS from apm_cli.utils.content_hash import compute_package_hash @@ -217,6 +218,49 @@ def _publish_legacy_plugin( ) +def _publish_agent_plugin( + scenario: _Scenario, + name: str, + *, + skill: str, +) -> _PublishedPackage: + source_root = scenario.isolated.package_root / name + skill_dir = source_root / "skills" / skill + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text(_skill(skill), encoding="ascii") + (source_root / "plugin.json").write_text( + json.dumps( + { + "$schema": PLUGIN_SCHEMA_ID, + "name": name, + "version": "1.0.0", + "description": "Agent Plugin target exclusion fixture", + }, + sort_keys=True, + ), + encoding="ascii", + ) + repository = scenario.repositories.create(name, source_tree=source_root) + commit = scenario.repositories.commit(repository, message="publish agent plugin") + remote_url = f"https://github.com/{_OWNER}/{name}" + return _PublishedPackage( + package=LocalPackage( + name=name, + root=source_root, + manifest_path=source_root / "plugin.json", + ), + repository=repository, + commit=commit, + remote_url=remote_url, + dependency={ + "git": remote_url, + "ref": commit.sha, + "alias": name, + }, + environment=scenario.repositories.url_rewrite_subprocess_env(repository, remote_url), + ) + + def _result_evidence(result: CommandResult) -> str: return ( f"cwd={result.cwd!s}\n" @@ -583,6 +627,81 @@ def test_required_invalid_skill_subset_never_reaches_manifest_or_lockfile( assert skill_subset_check["passed"] is False, _result_evidence(audit_result) +def test_required_agent_plugin_target_exclusion_noop_fails_without_mutating_state( + tmp_path: Path, + apm_binary_path: Path, +) -> None: + """Issue #2796: a target-excluded Agent Plugin is not a successful no-op.""" + scenario = _new_scenario(tmp_path / "agent-plugin-target-exclusion", apm_binary_path) + native = _publish_agent_plugin(scenario, "native-plugin", skill="native") + consumer = scenario.consumers.create( + "agent-plugin-noop-consumer", + dependencies=(native.dependency,), + targets=("codex",), + ) + capture_args = { + "targets": ("codex",), + "config_paths": ( + PurePosixPath("apm.lock.yaml"), + PurePosixPath(".codex/skills/native/SKILL.md"), + PurePosixPath(".agents/skills/native/SKILL.md"), + ), + } + before = LifecycleStateSnapshot.capture(consumer.root, **capture_args) + + install = scenario.runner.run( + (*_INSTALL_ARGS, "--target", "codex", "--skill", "native"), + scenario_id="agent-plugin-target-exclusion-noop", + cwd=consumer.root, + env=native.environment, + ) + after = LifecycleStateSnapshot.capture(consumer.root, **capture_args) + output = f"{install.stdout} {install.stderr}" + output_compact = "".join(output.split()) + expected_command = ( + f"apm install {_OWNER}/native-plugin/skills/native#{native.commit.sha} --target codex" + ) + + assert install.returncode == 1, _result_evidence(install) + assert "No selected target received this package" in output + assert "".join(expected_command.split()) in output_compact + assert "No changes" not in output + _assert_same_state(before, after) + + ordinary = _publish(scenario, "ordinary-after-native", skill="ordinary") + mixed_env = scenario.repositories.url_rewrite_subprocess_env_many( + ( + (native.repository, native.remote_url), + (ordinary.repository, ordinary.remote_url), + ) + ) + mixed_consumer = scenario.consumers.create( + "agent-plugin-mixed-consumer", + dependencies=(native.dependency, ordinary.dependency), + targets=("codex",), + ) + + mixed_install = _run_success( + scenario, + mixed_consumer, + (*_INSTALL_ARGS, "--target", "codex"), + environment=mixed_env, + scenario_id="agent-plugin-target-exclusion-mixed", + ) + mixed_output = f"{mixed_install.stdout} {mixed_install.stderr}" + + assert "No selected target received this package" in mixed_output + assert "".join(expected_command.split()) in "".join(mixed_output.split()) + assert not (mixed_consumer.root / ".codex" / "skills" / "native").exists() + lockfile = LockFile.read(mixed_consumer.root / "apm.lock.yaml") + assert lockfile is not None + locked_dependencies = lockfile.get_package_dependencies() + assert any( + dependency.name == "ordinary-after-native" and dependency.deployed_files + for dependency in locked_dependencies + ) + + def test_required_lsp_only_dry_run_reports_plan_without_writing_state( tmp_path: Path, apm_binary_path: Path, diff --git a/tests/unit/install/test_agent_plugin_deployment_boundary.py b/tests/unit/install/test_agent_plugin_deployment_boundary.py index 4743559a07..52eaa495eb 100644 --- a/tests/unit/install/test_agent_plugin_deployment_boundary.py +++ b/tests/unit/install/test_agent_plugin_deployment_boundary.py @@ -588,7 +588,7 @@ def test_target_exclusion_hint_names_remote_skill_subpath_form() -> None: AgentPluginTargetExcludedError("target excluded"), ) - assert "apm install kunchenguid/lavish-axi/skills/lavish --target codex" in message + assert "apm install kunchenguid/lavish-axi/skills/lavish#main --target codex" in message assert "#main/skills" not in message From 3b5059acb9ac1943e971fdb07fc43a5dd5692e39 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 4 Sep 2026 15:31:05 -0400 Subject: [PATCH 3/4] test: update plugin target no-op lifecycle Addresses the CI regression from the legacy exit-0 expectation by rewriting the Agent Plugin non-Copilot target test for issue #2796: a total target-exclusion no-op now fails and commits no durable state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../copilot_plugins/test_install_lifecycle.py | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/tests/unit/copilot_plugins/test_install_lifecycle.py b/tests/unit/copilot_plugins/test_install_lifecycle.py index 770dfd5a38..a3518127f0 100644 --- a/tests/unit/copilot_plugins/test_install_lifecycle.py +++ b/tests/unit/copilot_plugins/test_install_lifecycle.py @@ -124,16 +124,15 @@ def test_no_private_copilot_state_is_created( assert not (copilot_home / "settings.json").exists() -def test_non_copilot_target_skips_the_plugin_without_aborting( +def test_non_copilot_target_plugin_noop_fails_without_committing_state( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """A project that does not target copilot skips the plugin, exit 0. + """Issue #2796: target exclusion cannot be a successful no-op. Admission is a pure function of resolved target names -- a project-level - target set that excludes copilot is a non-applicability, not a failure. It - is skipped with one warning and the install still succeeds (Item 4). The - fatal path is reserved for a package that WAS selected for copilot but - cannot be registered. + target set that excludes copilot is a per-package skip, not a structural + package failure. When that skip leaves the command with nothing deployed, + install fails loudly and commits no durable state. """ project = tmp_path / "project" write_agent_plugin(tmp_path / "source" / "sentinel", name="sentinel") @@ -147,14 +146,11 @@ def test_non_copilot_target_skips_the_plugin_without_aborting( ) output = " ".join(result.output.split()) - assert result.exit_code == 0, result.output + assert result.exit_code == 1, result.output assert "'copilot' target" in output - materialized = _modules(project) / "_local" / "sentinel" - assert (materialized / "plugin.json").is_file() - lock = LockFile.from_yaml((project / "apm.lock.yaml").read_text(encoding="utf-8")) - locked = lock.get_package_dependencies() - assert len(locked) == 1 - assert locked[0].package_type == "agent_plugin" + assert "No selected target received this package" in output + assert "apm install" in output + assert not (project / "apm.lock.yaml").exists() assert not catalog_path_for(_modules(project)).exists() assert not ledger_path_for(_modules(project)).exists() assert not _settings_path(project).exists() From 9fc512786be6e0ca6fb079eb13964339af7a8f22 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Wed, 9 Sep 2026 10:58:52 +0200 Subject: [PATCH 4/4] fix: ground plugin recovery hints in declared skills Fold #2806 advisory follow-ups: select an inventoried skill directory, quote POSIX command operands, assert mixed-install skill bytes, and document dry-run behavior. Drop release-note duplicates resurrected while incorporating main. Mutation probes reject missing outcome, inventory and quoting guards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 15 +--- .../src/content/docs/reference/cli/install.md | 6 +- .../content/docs/reference/package-types.md | 15 ++-- .../.apm/skills/apm-usage/commands.md | 7 +- src/apm_cli/install/template.py | 29 +++---- .../test_required_lifecycle_state_machine.py | 16 +++- .../test_agent_plugin_deployment_boundary.py | 86 ++++++++++++++++++- 7 files changed, 125 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b6c565df3..4ef4721095 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,20 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- BREAKING: `apm install` now exits non-zero when target exclusion leaves an - Agent Plugins v1.0.0 package deployed to no target. Migration: install an - individual skill subpath such as - `apm install //skills/ --target ` (for example, - `apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`), or - select `--target copilot` for native registration; mixed installs still exit - 0 when at least one package deploys. (closes #2796) (#2806) -- Hermes is now a stable explicit-only target, and state-mutating APM commands - share one per-user cross-process lock to prevent concurrent updates from losing state. - (by @lkshrk; closes #2608) (#2655) -- Architecture ownership guards now use a sharded JSON registry and a - single-process Python linter while preserving exact-revision compatibility - and reducing warm median lint time by 75%. (#2739) -- This repository now pins the `copilot` target so plain `apm install` is deterministic across contributor environments. (by @tillig, #2771) +- **BREAKING:** Non-dry-run `apm install` now exits `1` when Agent Plugins v1 target exclusion leaves no package deployed; mixed installs that deploy another package still succeed. Install a direct skill subpath (`apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`) or select `--target copilot` for native registration. (closes #2796) (#2806) - **BREAKING:** after consumers re-vendor the shared gh-aw `apm.md`, its import requires an explicit `target` instead of deprecated `all`; `apm-action` otherwise writes `all` into the isolated `apm.yml`, where it degrades to auto-detection without harness markers. Set the workflow engine's target and recompile; see the [gh-aw migration recipe](https://microsoft.github.io/apm/integrations/gh-aw/#shared-apmmd-import-recommended). (#2706) - Re-vendored shared gh-aw workflows now default to APM 0.28.0 for both pack and restore, the version used for the recorded `microsoft/apm-action@v1.10.0` compatibility proof, not the latest CLI release; an explicit `apm-version` still overrides it. (#2706) diff --git a/docs/src/content/docs/reference/cli/install.md b/docs/src/content/docs/reference/cli/install.md index c6352fd4b4..7b092b560d 100644 --- a/docs/src/content/docs/reference/cli/install.md +++ b/docs/src/content/docs/reference/cli/install.md @@ -266,14 +266,14 @@ apm install owner/skill-bundle --skill '*' # reset to all skills | Code | Meaning | |---|---| -| `0` | Success. All requested dependencies and local content deployed. For Agent Plugins v1 packages, a mixed install still succeeds when target exclusion skips one package but at least one other package deploys. | -| `1` | Install failure: security scan blocked a critical finding, auth error, manifest or required MCP/LSP config write error, dependency resolution error, Agent Plugins v1 target exclusion left no package deployed, `--frozen` with a missing lockfile or a direct dependency absent from `apm.lock.yaml`, any reported install error (the diagnostic summary closes with `Installation failed with N error(s)`), or unhandled exception. `--force` does **not** suppress general install errors. The diagnostic summary names the cause. | +| `0` | Successful install or `--dry-run` preview. A preview does not certify real install success. For Agent Plugins v1 packages, a mixed install still succeeds when target exclusion skips one package but at least one other package deploys. | +| `1` | Install failure: security scan blocked a critical finding, auth error, manifest or required MCP/LSP config write error, dependency resolution error, Agent Plugins v1 target exclusion left no package deployed on a non-dry-run install, `--frozen` with a missing lockfile or a direct dependency absent from `apm.lock.yaml`, any reported install error (the diagnostic summary closes with `Installation failed with N error(s)`), or unhandled exception. `--force` does **not** suppress general install errors. The diagnostic summary names the cause. | | `2` | Usage error: no deployment target detectable (no `--target`, no `target(s):` in `apm.yml`, no default target configured via `apm config set target `, and no harness signal in the project), `--ssh` and `--https` both passed, `--frozen` and `--update` both passed, `--root` combined with `--global`, or a Click flag conflict. | ## Notes - **`--force` is dual-purpose.** It overwrites locally-authored files on collision **and** disables the critical-finding block from the built-in security scan. It does **not** suppress general install errors -- any error reported in the diagnostic summary still exits `1` (matches `npm` / `pip` / `cargo`). It does **not** refresh remote refs -- for routine ref updates, run [`apm update`](../update/). To remediate a blocked package, fix the reported source files and reinstall; `apm audit --strip` only remediates files that are already deployed. See [Drift and secure by default](../../../consumer/drift-and-secure-by-default/). -- **Agent Plugin target exclusion is fail-loud on total no-ops.** Agent Plugins v1 packages register natively only with the effective `copilot` target today. If every selected target excludes native registration and no other package deploys, install exits `1` and prints a skill-subpath recovery command such as `apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`, or you can select `--target copilot` for native registration. Mixed installs that deploy at least one other package still exit `0`. +- **Agent Plugin target exclusion is fail-loud on non-dry-run total no-ops.** Agent Plugins v1 packages register natively only with the effective `copilot` target today. If every selected target excludes native registration and no other package deploys, a non-dry-run install exits `1` and prints a skill-subpath recovery command such as `apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`. Keep refs after the skill path to install a plain skill bundle, or select `--target copilot` for native registration. Mixed installs that deploy at least one other package still exit `0`. For this target exclusion, `--dry-run` remains a successful preview without the per-package recovery diagnostic; it does not certify that a real install will succeed. - **Target contraction is reconciled.** A narrowed `targets:` in `apm.yml` is reconciled on the next non-dry-run install: deployed files, lockfile ownership, and merge-hook config/sidecar entries for the dropped target are cleaned up, even when no dependency itself changed. A package's own `target:` / `targets:` declaration applies an additional restriction within that effective set. See [Hooks and commands](../../../producer/author-primitives/hooks-and-commands/#hooks) for the full intersection and merge-hook config/sidecar details. `apm lock` may refresh the lockfile rows, but it never deletes deployed files from disk. - **Claude target prompt rewrite.** When deploying to `.claude/commands/`, prompt files with an `input:` front-matter key are rewritten to Claude's `arguments:` shape and `${input:name}` placeholders become `$name`. Argument names must match `^[A-Za-z][\w-]{0,63}$`; rejected names are dropped with a warning. - **MCP env-var passthrough.** Copilot CLI and Kiro translate `${env:VAR}` and `` to `${VAR}` in their MCP configs. Kiro writes `.kiro/settings/mcp.json` and `~/.kiro/settings/mcp.json` with `0o600` permissions. JetBrains Copilot preserves env references as `${env:VAR}` in `github-copilot/intellij/mcp.json`. Plaintext secrets are never written to disk for these runtime-resolved targets; legacy targets resolve placeholders at install time. diff --git a/docs/src/content/docs/reference/package-types.md b/docs/src/content/docs/reference/package-types.md index c41e17fa11..d81d11d1ce 100644 --- a/docs/src/content/docs/reference/package-types.md +++ b/docs/src/content/docs/reference/package-types.md @@ -298,15 +298,12 @@ dissects a recognized Agent Plugin through its normal primitive integrators. Older Copilot clients may copy plugins into private state outside APM ownership, so APM cannot guarantee cleanup of those client-created copies. -When target exclusion leaves a full Agent Plugins v1 package deployed to no -target, `apm install` exits `1` with a recovery command. To install one skill as -a plain skill bundle for another target, use -`apm install //skills/ --target `; refs stay after -the skill path, for example -`apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`. A mixed -install that deploys at least one other package still exits `0`. Publishers can -also provide a Claude-compatible package (`apm pack --claude-plugin`) for -non-Copilot targets. +On non-dry-run installs, target exclusion exits `1` only when no package +deploys; mixed installs that deploy another package still exit `0`. +`--dry-run` remains a successful preview, not proof of real install success. +See [`apm install` notes](../cli/install/#notes) for recovery commands and +dry-run diagnostic behavior. Publishers can also provide a Claude-compatible +package (`apm pack --claude-plugin`) for non-Copilot targets. ::: **When to choose:** you are producing a portable package with diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 778e64dba3..279ccb510b 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -153,10 +153,13 @@ writes only `.vscode/mcp.json`. `apm compile` continues to use legacy auto-detection with a `vscode`/`minimal` fallback for unsignalled projects -- bringing it onto the strict resolution chain is tracked as a follow-up. Agent Plugins v1 packages register natively only with the effective `copilot` -target today. If target exclusion leaves an install with no deployed package, +target today. If target exclusion leaves a non-dry-run install with no deployed package, `apm install` exits `1` and prints a recovery command such as `apm install kunchenguid/lavish-axi/skills/lavish#main --target codex`; mixed -installs that deploy at least one other package still exit `0`. +installs that deploy at least one other package still exit `0`. For this target +exclusion, `--dry-run` succeeds as a preview without the per-package recovery +diagnostic; it does not certify real install success. + ## Cache management | Command | Purpose | Key flags | diff --git a/src/apm_cli/install/template.py b/src/apm_cli/install/template.py index 5466febee0..76ffb1f57c 100644 --- a/src/apm_cli/install/template.py +++ b/src/apm_cli/install/template.py @@ -13,6 +13,7 @@ from __future__ import annotations +import shlex from pathlib import Path from typing import TYPE_CHECKING, cast @@ -273,21 +274,15 @@ def _agent_plugin_skill_name_for_hint( materialization: Materialization, ) -> str | None: """Return one skill name that can be installed through the subpath route.""" - requested = getattr(source.ctx, "skill_subset", None) - if requested: - for value in requested: - skill_name = str(value) - if skill_name and skill_name != "*": - return skill_name - package = getattr(materialization.package_info, "package", None) plugin = getattr(package, "agent_plugin", None) - components = getattr(plugin, "components", None) - for skill in getattr(components, "skills", ()) or (): - skill_name = getattr(skill, "directory_name", None) or getattr(skill, "name", None) - if skill_name: - return str(skill_name) - return None + if plugin is None or not plugin.components.skills: + return None + requested = frozenset(getattr(source.ctx, "skill_subset", None) or ()) + for skill in plugin.components.skills: + if skill.directory_name in requested or skill.name in requested: + return skill.directory_name + return plugin.components.skills[0].directory_name def _agent_plugin_target_skip_message( @@ -306,11 +301,13 @@ def _agent_plugin_target_skip_message( dep_base, ref_suffix = _dependency_parts_for_skill_subpath(source.dep_ref) target_arg = _target_names_for_hint(source.ctx) - command = f"apm install {dep_base}/skills/{skill_name}{ref_suffix} --target {target_arg}" + dependency = f"{dep_base}/skills/{skill_name}{ref_suffix}" + command = f"apm install {shlex.quote(dependency)} --target {shlex.quote(target_arg)}" + shell_note = " (POSIX shell)" if shlex.quote(dependency) != dependency else "" return ( f"{message} No selected target received this package. " - "To install this skill as a plain skill bundle, use " - f"'{command}'." + f"To install available skill '{skill_name}' as a plain skill bundle, " + f"use{shell_note}: {command}" ) diff --git a/tests/integration/test_required_lifecycle_state_machine.py b/tests/integration/test_required_lifecycle_state_machine.py index 359c86c2ae..90f13bcf97 100644 --- a/tests/integration/test_required_lifecycle_state_machine.py +++ b/tests/integration/test_required_lifecycle_state_machine.py @@ -665,7 +665,7 @@ def test_required_agent_plugin_target_exclusion_noop_fails_without_mutating_stat output = f"{install.stdout} {install.stderr}" output_compact = "".join(output.split()) expected_command = ( - f"apm install {_OWNER}/native-plugin/skills/native#{native.commit.sha} --target codex" + f"apm install '{_OWNER}/native-plugin/skills/native#{native.commit.sha}' --target codex" ) assert install.returncode == 1, _result_evidence(install) @@ -706,6 +706,20 @@ def test_required_agent_plugin_target_exclusion_noop_fails_without_mutating_stat dependency.name == "ordinary-after-native" and dependency.deployed_files for dependency in locked_dependencies ) + ordinary_dependency = next( + dependency + for dependency in locked_dependencies + if dependency.name == "ordinary-after-native" + ) + ordinary_skills = [ + path for path in ordinary_dependency.deployed_files if path.endswith("/SKILL.md") + ] + assert ordinary_skills + expected_skill = ordinary.package.root / "skills" / "ordinary" / "SKILL.md" + for relative_path in ordinary_skills: + deployed_skill = mixed_consumer.root / relative_path + assert deployed_skill.is_file() + assert deployed_skill.read_bytes() == expected_skill.read_bytes() def test_required_lsp_only_dry_run_reports_plan_without_writing_state( diff --git a/tests/unit/install/test_agent_plugin_deployment_boundary.py b/tests/unit/install/test_agent_plugin_deployment_boundary.py index 52eaa495eb..04037242ef 100644 --- a/tests/unit/install/test_agent_plugin_deployment_boundary.py +++ b/tests/unit/install/test_agent_plugin_deployment_boundary.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import shlex from pathlib import Path from types import SimpleNamespace from unittest.mock import MagicMock @@ -568,19 +569,32 @@ def tracked_integrate(self, package_info, *args, **kwargs): assert not (project / ".agents" / "skills" / "native").exists() -def test_target_exclusion_hint_names_remote_skill_subpath_form() -> None: +@pytest.mark.parametrize("requested", (("lavish",), ("missing",), ("safe",), ("*",), ())) +def test_target_exclusion_hint_names_remote_skill_subpath_form( + requested: tuple[str, ...], +) -> None: """The no-op error must name the working subpath workaround from #2796.""" source = SimpleNamespace( ctx=SimpleNamespace( targets=[SimpleNamespace(name="codex")], - skill_subset=("lavish",), + skill_subset=requested, ), dep_ref=SimpleNamespace( is_local=False, to_display_reference=lambda: "kunchenguid/lavish-axi#main", ), ) - materialization = SimpleNamespace(package_info=SimpleNamespace(package=None)) + materialization = SimpleNamespace( + package_info=SimpleNamespace( + package=SimpleNamespace( + agent_plugin=SimpleNamespace( + components=SimpleNamespace( + skills=(SimpleNamespace(directory_name="lavish", name="lavish"),) + ) + ) + ) + ) + ) message = _agent_plugin_target_skip_message( source, @@ -588,10 +602,74 @@ def test_target_exclusion_hint_names_remote_skill_subpath_form() -> None: AgentPluginTargetExcludedError("target excluded"), ) - assert "apm install kunchenguid/lavish-axi/skills/lavish#main --target codex" in message + assert "apm install 'kunchenguid/lavish-axi/skills/lavish#main' --target codex" in message assert "#main/skills" not in message +@pytest.mark.parametrize( + "dependency", + ("owner/plugin#feature;echo-surprise", "owner/plugin#feature'quoted", "../plugin with spaces"), +) +def test_target_exclusion_hint_quotes_the_complete_dependency_argument( + tmp_path: Path, + dependency: str, +) -> None: + package_info = _write_adversarial_agent_plugin(tmp_path / "native", tmp_path / "outside") + source = SimpleNamespace( + ctx=SimpleNamespace(targets=[SimpleNamespace(name="codex")], skill_subset=("native",)), + dep_ref=SimpleNamespace( + is_local=dependency.startswith("."), + local_path=dependency if dependency.startswith(".") else None, + to_display_reference=lambda: dependency, + ), + ) + message = _agent_plugin_target_skip_message( + source, + SimpleNamespace(package_info=package_info), + AgentPluginTargetExcludedError("target excluded"), + ) + base, separator, ref = dependency.partition("#") + command = message.split("use (POSIX shell): ", 1)[1] + tokens = shlex.shlex(command, posix=True, punctuation_chars=";&|<>()") + tokens.whitespace_split = True + tokens.commenters = "" + assert list(tokens) == [ + "apm", + "install", + f"{base}/skills/native{separator}{ref}", + "--target", + "codex", + ] + + +def test_target_exclusion_hint_matches_skill_name_but_uses_inventory_directory( + tmp_path: Path, +) -> None: + package_info = _write_adversarial_agent_plugin(tmp_path / "native", tmp_path / "outside") + plugin = package_info.package.agent_plugin + assert plugin is not None + from dataclasses import replace + + first = plugin.components.skills[0] + package_info.package.agent_plugin = replace( + plugin, + components=replace( + plugin.components, + skills=(first, replace(first, directory_name="other-directory", name="selected")), + ), + ) + source = SimpleNamespace( + ctx=SimpleNamespace(targets=[SimpleNamespace(name="codex")], skill_subset=("selected",)), + dep_ref=SimpleNamespace(is_local=False, to_display_reference=lambda: "owner/plugin#v1"), + ) + message = _agent_plugin_target_skip_message( + source, + SimpleNamespace(package_info=package_info), + AgentPluginTargetExcludedError("excluded"), + ) + assert "apm install 'owner/plugin/skills/other-directory#v1' --target codex" in message + + @pytest.mark.parametrize("include_ordinary", (False, True)) def test_dry_run_target_exclusion_is_non_fatal_preview( tmp_path: Path,