diff --git a/CHANGELOG.md b/CHANGELOG.md index fc6df1ff7f..4ef4721095 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **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 f691690a6e..7b092b560d 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` | 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 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 11e0c774ac..d81d11d1ce 100644 --- a/docs/src/content/docs/reference/package-types.md +++ b/docs/src/content/docs/reference/package-types.md @@ -296,9 +296,14 @@ 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. + +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 1ec5ae5235..279ccb510b 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -152,6 +152,14 @@ 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 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`. 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/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..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 @@ -118,9 +119,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 +147,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 @@ -242,17 +245,83 @@ 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_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"): + display = dep_ref.to_display_reference() + elif hasattr(dep_ref, "get_identity"): + display = dep_ref.get_identity() + else: + display = getattr(dep_ref, "repo_url", "") + base, separator, reference = str(display).partition("#") + return base.rstrip("/"), f"{separator}{reference}" if separator else "" + + +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.""" + package = getattr(materialization.package_info, "package", None) + plugin = getattr(package, "agent_plugin", 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( + 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, ref_suffix = _dependency_parts_for_skill_subpath(source.dep_ref) + target_arg = _target_names_for_hint(source.ctx) + 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. " + f"To install available skill '{skill_name}' as a plain skill bundle, " + f"use{shell_note}: {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/integration/test_required_lifecycle_state_machine.py b/tests/integration/test_required_lifecycle_state_machine.py index d4c35f4e0e..90f13bcf97 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 @@ -223,6 +224,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" @@ -589,6 +633,95 @@ 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 + ) + 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( tmp_path: Path, apm_binary_path: Path, diff --git a/tests/spec_conformance/test_manifest_reqs.py b/tests/spec_conformance/test_manifest_reqs.py index 47c4f70c40..a4c12ad879 100644 --- a/tests/spec_conformance/test_manifest_reqs.py +++ b/tests/spec_conformance/test_manifest_reqs.py @@ -1121,7 +1121,7 @@ def test_agent_plugin_target_exclusion_materializes_without_projection( ) -> None: """Bind req-tg-011 to the real excluded-target install lifecycle.""" from tests.unit.copilot_plugins.test_install_lifecycle import ( - test_non_copilot_target_skips_the_plugin_without_aborting as _run_boundary_contract, + test_non_copilot_target_plugin_noop_fails_without_committing_state as _run_boundary_contract, ) _run_boundary_contract(tmp_path, monkeypatch) 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() diff --git a/tests/unit/install/test_agent_plugin_deployment_boundary.py b/tests/unit/install/test_agent_plugin_deployment_boundary.py index 7eba212127..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 @@ -31,7 +32,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 +297,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 +305,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 +366,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 +445,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 +503,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 +522,167 @@ 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() + + +@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=requested, + ), + dep_ref=SimpleNamespace( + is_local=False, + to_display_reference=lambda: "kunchenguid/lavish-axi#main", + ), + ) + 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, + materialization, + AgentPluginTargetExcludedError("target excluded"), + ) + + 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_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 +723,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(