fix(install): fail loudly when a package deploys to no target (closes #2796) - #2806
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5ec924f to
444636e
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes correctly implement the specified failure condition with targeted diagnostics and comprehensive unit coverage for the new behavior (pure no-op, mixed batch, and dry-run).
Review tier: Lite
Findings: None
What changed in this PR
This PR updates the apm install outcome logic so that an Agent Plugins v1.0.0 package that is excluded by target selection (i.e., no selected target can receive it) is no longer treated as a successful no-op. Instead, the install fails (non-zero exit) only when that exclusion results in zero packages being deployed, matching the intent described in issue #2796.
Changes:
- Add a dedicated diagnostic subtype/count for “Agent Plugin target excluded” warnings and plumb it into install outcome classification.
- Update the install outcome owner to fail when
installed_count == 0and at least one Agent Plugin was target-excluded. - Expand/adjust unit tests to cover: pure-no-op failure, mixed-batch non-fatal behavior, dry-run preview remaining non-fatal, and the actionable “install via /skills/” hint.
| File | Description |
|---|---|
| tests/unit/install/test_agent_plugin_deployment_boundary.py | Updates and adds regression tests for the new “fail loudly on pure no-op” behavior, including hint wording and dry-run parity. |
| src/apm_cli/utils/diagnostics.py | Extends DiagnosticCollector.warn() with a severity subtype and adds a first-class Agent Plugin target-exclusion warning + counter. |
| src/apm_cli/install/template.py | Builds an actionable skip message (including /skills/<skill> workaround) and records target exclusion using the new diagnostic subtype. |
| src/apm_cli/install/outcome.py | Adds agent_plugin_target_excluded_count() helper and fails the install when exclusion caused a zero-deploy run. |
| CHANGELOG.md | Documents the breaking change and migration guidance under [Unreleased]. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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>
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>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Prior architecture finding resolved. Canonical inventory drives recovery; the outcome owner preserves mixed success and rejects zero-deployment exclusion. |
| CLI Logging Expert | 0 | 0 | 0 | No logging concerns: total exclusion reports failure; mixed success and inventory-grounded, ref-preserving recovery remain intact. |
| DevX UX Expert | 0 | 0 | 0 | Prior recovery finding resolved: inventory-backed hints preserve refs; total exclusion fails, mixed success remains, and dry-run behavior is documented. |
| Supply Chain Security | 0 | 0 | 0 | Complete-argument quoting resolves prior feedback; total exclusion fails before cleanup, while mixed success and scoped rollback remain intact. |
| OSS Growth Hacker | 0 | 0 | 0 | Recovery is inventory-grounded and copyable; documentation explains failed total exclusions, successful mixed installs, and dry-run limits. No growth concerns remain. |
| Doc Writer | 0 | 0 | 0 | Prior documentation follow-up resolved: non-dry-run failure, successful mixed batches, direct-skill recovery, and preview limits are documented. |
| Test Coverage | 0 | 0 | 0 | Previous artifact gap is resolved; no-op, mixed-success, inventory and quoting regressions have targeted assertions. Exact-head execution remains parent-owned. |
| Performance Expert | 0 | 0 | 0 | No scoped performance regression found: inventory lookup uses set membership, and exclusion counting remains linear at outcome boundaries. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
direction LR
class DependencySource {
<<Strategy>>
+acquire() Materialization
}
class LocalDependencySource
class CachedDependencySource
class FreshDependencySource
DependencySource <|-- LocalDependencySource
DependencySource <|-- CachedDependencySource
DependencySource <|-- FreshDependencySource
class Materialization {
<<Dataclass>>
+package_info
+deltas
}
class InstallContext {
+installed_count
+skill_subset
+diagnostics
}
class InstallTemplate {
<<Module>>
+run_integration_template()
+_record_agent_plugin_target_skip()
+_agent_plugin_skill_name_for_hint()
}
note for InstallTemplate "Template Method: shared post-acquisition integration in install/template.py"
class AgentPlugin {
<<ImmutableIR>>
+components
}
class AgentPluginComponents {
+skills
}
class AgentPluginSkill {
+directory_name
+name
}
class DiagnosticCollector {
+agent_plugin_target_excluded()
+agent_plugin_target_excluded_count
+render_summary()
}
class Diagnostic {
<<Dataclass>>
+category
+severity
+message
}
note for DiagnosticCollector "Collect-then-render: typed exclusion fact, aggregate classification, then presentation"
class InstallOutcome {
<<Module>>
+result_from_install_context()
+finalize_install_result()
+apply_install_command_outcome()
}
class InstallResult {
<<Dataclass>>
+installed_count
+disposition
+exit_code
}
class InstallTransaction {
+complete()
+commit()
+rollback()
}
DependencySource --> InstallContext
DependencySource ..> Materialization : returns
Materialization ..> AgentPlugin : package_info.package.agent_plugin
AgentPlugin *-- AgentPluginComponents
AgentPluginComponents *-- AgentPluginSkill
InstallTemplate ..> Materialization
InstallTemplate ..> AgentPluginSkill : reads canonical inventory
InstallTemplate ..> DiagnosticCollector : records exclusion
InstallContext --> DiagnosticCollector
DiagnosticCollector *-- Diagnostic
InstallOutcome ..> InstallContext
InstallOutcome ..> DiagnosticCollector : reads facts
InstallOutcome ..> InstallResult : owns classification
InstallTransaction ..> InstallResult : consumes disposition
class InstallTemplate:::touched
class InstallOutcome:::touched
class DiagnosticCollector:::touched
class Diagnostic:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm install: install()<br/>src/apm_cli/commands/install.py"] --> B["run_install_pipeline()<br/>src/apm_cli/install/pipeline.py"]
B --> D{"dry-run?"}
D -->|yes| P["[I/O] preflight_agent_plugin_dry_run()<br/>install/template.py<br/>Target exclusion is swallowed; structural errors still propagate"]
P --> PV["DRY_RUN preview: exit 0 for target exclusion<br/>No recovery diagnostic or certified deployment"]
D -->|no| AC["[NET] [FS] download.run() and source.acquire()<br/>install/phases/download.py; install/sources.py"]
AC --> PF["preflight_agent_plugin_materializations()<br/>install/template.py<br/>Target exclusion does not abort the batch"]
PF --> T["run_integration_template() -> _integrate_materialization()<br/>install/template.py: each dependency"]
T --> E{"enforce_agent_plugin_deployment_boundary()<br/>agent_plugins/errors.py"}
E -->|target excluded| S["_record_agent_plugin_target_skip()<br/>installed delta = 0; deployed_files = []"]
S --> H["_agent_plugin_skill_name_for_hint()<br/>install/template.py<br/>Read AgentPlugin.components.skills"]
H --> HS{"inventoried skill available?"}
HS -->|yes| HC["_agent_plugin_target_skip_message()<br/>Match requested name or directory; otherwise available alternative<br/>Quote complete skill path plus retained ref"]
HS -->|no| HF["_agent_plugin_target_skip_message()<br/>Copilot or target-compatible-package fallback"]
HC --> W["DiagnosticCollector.agent_plugin_target_excluded()<br/>utils/diagnostics.py"]
HF --> W
E -->|structural error| ER["_record_agent_plugin_boundary_failure()<br/>Record error; installed delta = 0"]
E -->|admitted or ordinary| I["[FS] integrate_package_primitives()<br/>install/services.py<br/>Record deployment deltas"]
W --> AG["integrate.run()<br/>install/phases/integrate.py<br/>Aggregate installed_count across batch"]
ER --> AG
I --> AG
AG --> O["result_from_install_context() -> finalize_install_result()<br/>install/outcome.py"]
O --> Q{"Existing failure condition OR<br/>installed_count == 0 and exclusion_count > 0?"}
Q -->|yes| F["FAILED: return before cleanup/finalize"]
Q -->|no| C["[FS] [LOCK] Remaining pipeline phases<br/>install/phases/finalize.py: finalize.run()"]
F --> RB["[FS] [LOCK] InstallTransaction.complete()<br/>install/transaction.py: rollback"]
C --> CM["[FS] [LOCK] InstallTransaction.complete()<br/>Commit successful result"]
RB --> R["[I/O] render_post_install_summary()<br/>install/summary.py<br/>Consume canonical disposition"]
CM --> R
R --> X{"apply_install_command_outcome()<br/>install/outcome.py"}
X -->|FAILED| X1["exit 1; success summary disallowed"]
X -->|SUCCESS or PARTIAL_SUCCESS| X0["exit 0; successful mixed batch preserved"]
Recommendation
Recommend shipping the reviewed code with no remaining code follow-ups. This advisory stance is not policy approval or proof of current CI success. The parent should establish exact-head validation evidence for the maintainer's shipping decision; unexpected failures warrant reassessment, not speculative findings now.
Full per-persona findings
Python Architect
No findings.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
Complete exact-head diff fallback check confirms src/apm_cli/install/outcome.py, src/apm_cli/install/template.py, src/apm_cli/utils/diagnostics.py and accompanying tests/docs change outcomes and recovery hints, not credentials, host classification or authentication semantics.
Doc Writer
No findings.
Test Coverage
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
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>
Shepherd completion: code complete, required review outstandingHead Reservations carried from strategic alignment
Folded in this run
Copilot signals reviewedTwo inline-comment reads returned no findings. The existing Copilot review is Regression and lifecycle evidence
Canonical owner and lintDeterministic detection finds one touched owner: Install success / failure The complete current CI lint contract passed locally before push: ruff pair, CI and mergeabilityCurrent CI includes
Two outer iterations; zero CI-recovery iterations. Source, schemas, ownership, |
Context: Agent Plugins v1.0.0 packages can only register natively with the copilot target today. When every selected target was non-copilot,
apm install --skill ... --target codexskipped the package, deployed nothing, printedNo changes, and exited 0.WHY: A successful no-op hid an unsatisfied install request. The install outcome now fails only when target exclusion leaves the run with zero deployed packages, while mixed installs that deploy at least one package still exit 0.
BREAKING CHANGE:
apm installnow exits non-zero when Agent Plugin target exclusion means no package was deployed. Migration: install a direct skill subpath such asapm install kunchenguid/lavish-axi/skills/lavish#main --target codex, or select--target copilotfor native Agent Plugin registration.Scenario Evidence:
tests/integration/test_required_lifecycle_state_machine.py::test_required_agent_plugin_target_exclusion_noop_fails_without_mutating_stateAPM_E2E_TESTS=1 uv run --extra dev pytest tests/integration/test_required_lifecycle_state_machine.py::test_required_agent_plugin_target_exclusion_noop_fails_without_mutating_state -q->1 passed in 2.50stests/unit/install/test_agent_plugin_deployment_boundary.py::test_target_exclusion_does_not_hard_fail_when_the_rest_of_the_batch_installsuv run --extra dev pytest tests/unit/install/test_agent_plugin_deployment_boundary.py::test_target_exclusion_hint_names_remote_skill_subpath_form tests/unit/install/test_agent_plugin_deployment_boundary.py::test_target_exclusion_fails_when_no_package_is_deployed tests/unit/install/test_agent_plugin_deployment_boundary.py::test_target_exclusion_does_not_hard_fail_when_the_rest_of_the_batch_installs -q->10 passed in 2.44sHow to test:
uv run --extra dev pytest tests/unit/install/test_agent_plugin_deployment_boundary.py::test_target_exclusion_hint_names_remote_skill_subpath_form tests/unit/install/test_agent_plugin_deployment_boundary.py::test_target_exclusion_fails_when_no_package_is_deployed tests/unit/install/test_agent_plugin_deployment_boundary.py::test_target_exclusion_does_not_hard_fail_when_the_rest_of_the_batch_installs -quv run --extra dev pytest tests/unit/copilot_plugins/test_install_lifecycle.py -qAPM_E2E_TESTS=1 uv run --extra dev pytest tests/integration/test_required_lifecycle_state_machine.py::test_required_agent_plugin_target_exclusion_noop_fails_without_mutating_state -quv run --extra dev ruff check src/ tests/uv run --extra dev ruff format --check src/ tests/Architecture: owner-extension. The deterministic owner-touch gate reports
src/apm_cli/install/outcome.pyunder the canonical decisionInstall success / failure outcome; this PR extends that existing owner with the target-excluded no-deploy failure case while keeping per-package target exclusion detection in the Agent Plugin deployment boundary and diagnostics collector.apm-spec-waiver: APM CLI-specific Agent Plugin target-exclusion UX; OpenAPM manifest and registry semantics are unchanged.
closes #2796