Fix: make --skill deployment additive, not just persistence (#1786 follow-up)

[danielmeppiel]

TL;DR

PR #1786 made apm.yml/lockfile persistence of --skill subsets additive, but the deployment path still used the raw CLI subset. A second apm install <bundle> --skill B deployed only B and the stale-file cleanup deleted the previously deployed skill A from the target folder — so the manifest said [A, B] while the disk had only B, until the next bare apm install. This PR closes that gap so deployment, lockfile, and manifest stay in lockstep.

Problem (WHY)

apm install <bundle> --skill A then apm install <bundle> --skill B should leave both skills deployed (B installed on top of A). Instead:

• apm.yml correctly merged to [A, B] (the Fix --skill to merge additively with existing skills in apm.yml #1786 fix), but
• deployment used the raw CLI subset (B), and stale-file cleanup removed A from disk.

The result was a silent divergence between the manifest/lockfile and the actual on-disk deployment.

Approach (WHAT)

Deploy the union of the persisted manifest skills: and the current CLI --skill values, and make --skill '*' honor its documented "reset to all skills" contract on the manifest as well as on disk.

Implementation (HOW)

• New shared helper effective_deploy_skill_subset() (install/package_resolution.py): returns the sorted, deduped union of the persisted subset and the CLI subset; returns None (deploy all) for --skill '*'.
• install/template.py: deployment now calls the helper instead of using the raw CLI subset.
• install/phases/lockfile.py: _attach_skill_subset_override now unions the CLI values with the persisted subset (via the helper) so the lockfile records what was actually deployed, instead of clobbering with the raw CLI value.
• commands/install.py: --skill '*' now resets the apm.yml skills: pin back to the full bundle (drops the field → plain string entry), matching the documented contract (--skill help text and migration.md: "back to full bundle"). Threads a skill_subset_from_cli flag through _validate_and_add_packages_to_apm_yml → _resolve_package_references.

Trade-offs

• The helper unions CLI with the persisted manifest subset (manifest = source of truth), not the previous lockfile subset — so skills a user manually removes from apm.yml are not silently re-added. Unioning the just-requested CLI skill is a safety net so the requested skill always deploys.

Validation evidence

• New regression suite tests/unit/install/test_skill_subset_deploy_parity.py (11 tests): helper union/reset unit tests + e2e for additive deploy-on-top, bare reinstall, --skill '*' reset, manifest-reduction cleanup, and uninstall-removes-union.
• Full suites green: tests/unit/test_skill_subset_persistence.py, tests/unit/commands/test_install_skill_subset.py, tests/unit/install/ (1759 passed); command/policy install+lockfile+skill tests (272 passed).
• CI-mirror lint chain clean: ruff check, ruff format --check, pylint R0801 (10.00/10), lint-auth-signals.sh; all changed files well under the file-length guard.

Live end-to-end (separate processes)

1) +cosmos-db       disk=[cosmos-db]                manifest=[cosmos-db]
2) +functions       disk=[cosmos-db, functions]     manifest=[cosmos-db, functions]   <- additive fix
3) bare reinstall   disk=[cosmos-db, functions]
4) --skill '*'      disk=[aks, cosmos-db, functions] manifest skills: reset (full bundle)
5) bare after '*'   disk=[aks, cosmos-db, functions]  (no re-pruning)
6) manifest -> [cosmos-db] + bare  disk=[cosmos-db]   (stale cleanup)
7) uninstall bundle disk=[]

How to test

uv run --extra dev python -m pytest tests/unit/install/test_skill_subset_deploy_parity.py -q

Or manually: install a multi-skill bundle with --skill A, then --skill B, and confirm both land in .agents/skills/ (or your target) and in apm.lock.yaml's skill_subset.

---

apm-spec-waiver: bug fix to apm-CLI --skill additive deploy parity; no net-new OpenAPM wire-format surface -- lockfile skill_subset field format and semantics are unchanged, and #1786 established the additive --skill contract without a spec requirement, so no anchor/manifest row applies.

[danielmeppiel]

APM Review Panel: ship_with_followups

Fixes silent skill-data deletion on additive --skill installs by unifying deploy/lockfile resolution against the persisted manifest union -- restoring the trust contract that installed artifacts survive across invocations.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All nine panelists converge: this is a correctness fix that closes a silent-data-loss regression introduced by #1786. The test-coverage-expert confirms 11 passing e2e regression traps at the install-pipeline tier -- the correct floor for a deploy-path integrity fix. The test evidence is load-bearing: additive deploy, bare reinstall, star reset, manifest reduction, and uninstall scenarios all hold on this commit. No panelist raised a blocking finding.

The strongest cross-panelist signal is the skill_subset_from_cli=True hardcode at lockfile.py:249, independently flagged by python-architect (recommended) and supply-chain-security (nit). Both agree the intent is correct today but the hardcode creates a defensive-drift trap if _attach_skill_subset_override is ever invoked outside a CLI-driven flow. The fix (bool(self.ctx.skill_subset) or threading the context flag) is trivial and should land as a fast follow. Beyond that, the cli-logging and doc-writer findings are real UX polish gaps -- users deserve a breadcrumb confirming which skills were actually deployed and a doc clause stating additive semantics -- but neither blocks ship.

Strategically, this PR is table-stakes trust infrastructure: a package manager that silently deletes previously-installed artifacts on a subsequent install loses credibility instantly. Shipping this quickly reinforces APM's npm-parity mental model and ensures our additive --skill story is defensible.

Dissent. No material dissent. python-architect and supply-chain-security converged on the same lockfile.py:249 concern from different angles (architectural contract vs future-caller threat surface); both agree it is non-blocking. devx-ux and cli-logging each want different output hooks but do not contradict each other.

Aligned with: Pragmatic as npm (additive --skill never removes an existing skill; * resets); Portable by manifest (deploy resolution draws from the persisted apm.yml pin, not transient CLI state); Secure by default (restores the invariant that installed content survives unless explicitly removed).

Growth signal. Trust-protecting bugfix worth framing in CHANGELOG as a user-visible promise restoration, not an implementation detail. The additive mental model + * escape hatch is a clean release-note beat ("apm install --skill is now npm-like: additive by default, reset with *").

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	Clean extraction of effective_deploy_skill_subset as single-source-of-truth for deploy resolution; minor redundancy with normalize_and_merge_skill_subset.
CLI Logging Expert	0	2	1	Additive deploy and --skill '*' reset are correct but silent; users get no breadcrumb of what was deployed.
DevX UX Expert	0	1	2	Additive --skill matches npm mental model; no ergonomic regression; removal path under-documented.
Supply Chain Security	0	0	1	Union draws from user-controlled manifest (not stale lockfile); deploy/lockfile parity holds; no surface expansion. Ship.
OSS Growth Hacker	0	0	3	Trust-protecting bugfix; frame CHANGELOG + docs callout around the additive promise.
Doc Writer	0	1	0	No doc drift; * reset contract now honored by code; add one clause that --skill is additive across installs.
Test Coverage Expert	0	0	1	All five regression scenarios have passing e2e traps (11 passed in 1.79s); ship.
Performance Expert	0	0	1	Pure set-arithmetic over tiny skill lists; no new I/O or materialization overhead. Ship.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Python Architect + Supply Chain Security] Replace hardcoded skill_subset_from_cli=True at lockfile.py:249 with self.ctx.skill_subset_from_cli -- Convergent signal from two independent panelists. Prevents a defensive-drift trap if the lockfile phase is ever invoked outside CLI-driven install.
2. [Doc Writer] Add one clause to reference/cli/install.md and apm-usage/commands.md stating --skill is additive across separate installs (union, not replace) -- Users discovering --skill in the docs today have no way to know the additive contract without reading source.
3. [CLI Logging Expert] Emit a user-facing message on additive --skill deploy confirming which skills were retained and on * reset confirming pin removal -- Silent correctness is still surprising; a breadcrumb builds trust and aids debugging.
4. [OSS Growth Hacker] Frame the CHANGELOG entry as a user-visible promise rather than implementation detail -- CHANGELOG is a public trust surface; leading with the user promise reinforces positioning.
5. [DevX UX Expert] Document how to REMOVE a single previously-pinned skill in help text -- Additive semantics require a discoverable removal path.

Architecture

classDiagram
    direction LR
    class InstallContext {
        +skill_subset tuple~str~ or None
        +skill_subset_from_cli bool
    }
    class effective_deploy_skill_subset {
        +__call__(skill_subset_from_cli, cli_subset, persisted_subset) tuple or None
    }
    class TemplateIntegrator {
        +_integrate_materialization(ctx, dep_ref)
    }
    class LockfileBuildPhase {
        +_attach_skill_subset_override(lockfile)
    }
    InstallContext ..> effective_deploy_skill_subset : provides inputs
    TemplateIntegrator ..> effective_deploy_skill_subset : calls
    LockfileBuildPhase ..> effective_deploy_skill_subset : calls
    class effective_deploy_skill_subset:::touched
    class LockfileBuildPhase:::touched
    class TemplateIntegrator:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    CLI["apm install bundle --skill B"]
    CLI --> NORM["normalize_and_merge_skill_subset"]
    NORM -->|"union(CLI B, persisted A)"| MANIFEST["[FS] apm.yml skills: [A,B]"]
    CLI --> DEPLOY_RESOLVE["effective_deploy_skill_subset"]
    DEPLOY_RESOLVE -->|"union(persisted [A,B], CLI (B,))"| DEPLOY["[FS] deploy skills A + B"]
    CLI --> LOCK["_attach_skill_subset_override"]
    LOCK -->|"effective_deploy_skill_subset"| LOCKW["[FS] apm.lock.yaml skill_subset [A,B]"]
    DEPLOY --> STALE["[FS] stale cleanup removes only files NOT in union"]

Loading

Recommendation

Merge after folding the in-scope follow-ups. The PR restores a broken trust invariant (no silent artifact deletion), is backed by 11 passing e2e regression traps at the correct tier, and carries zero blocking findings from nine panelists. The author is folding the lockfile.py:249 defensive hardening, the doc clause, the CLI breadcrumbs, help-text removal path, and CHANGELOG framing into this PR before merge.

_{This panel is advisory. It does not block merge.}

[Copilot]

Pull request overview

This PR closes a parity gap introduced after #1786 by making the deployment and lockfile recording paths for --skill behave additively (union with the persisted manifest subset), so the manifest/lockfile and on-disk deployed skills don’t silently diverge across repeated installs.

Changes:

• Add effective_deploy_skill_subset() to compute the union of persisted + CLI skill subsets (and interpret --skill '*' as “deploy all”).
• Switch deployment (install/template.py) and lockfile override (install/phases/lockfile.py) to use the effective union, keeping deploy and lockfile aligned with persisted state.
• Thread a skill_subset_from_cli flag into install validation/resolution and add regression coverage for deploy parity.

Show a summary per file

File	Description
tests/unit/install/test_skill_subset_deploy_parity.py	Adds regression tests for additive deploy-on-top behavior, --skill '*' reset, manifest-driven reduction cleanup, and uninstall behavior.
src/apm_cli/install/template.py	Uses the new helper so deployment uses the effective union (rather than raw CLI subset).
src/apm_cli/install/phases/lockfile.py	Updates lockfile skill subset override to union CLI subset with persisted subset via the helper.
src/apm_cli/install/package_resolution.py	Introduces effective_deploy_skill_subset() helper to centralize deploy-subset resolution semantics.
src/apm_cli/commands/install.py	Threads skill_subset_from_cli and resets persisted skills: pin on --skill '*' (when resolving explicit package refs).

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Low

[danielmeppiel]

APM Review Panel: ship_now

Fixes silent skill data-loss on successive apm install --skill calls by making deployment additive, matching the persistence contract established in #1786.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All nine panelists converge: this is a correctness fix for a silent data-loss regression with no security, auth, or performance concerns. The additive-union helper (effective_deploy_skill_subset()) sources its truth from the manifest -- not the lockfile -- which supply-chain-security confirms avoids any re-introduction vector for untrusted skills. The helper extraction to package_resolution.py satisfies SRP and the 2100-line CI cap without introducing new abstraction debt.

The test-coverage-expert confirmed all 17 tests pass (up from 11 in round 1) and both unit-test gaps flagged in round 1 were addressed this round with dedicated tests for the reset and no-op branches. Doc-writer's in-scope recommendations (package-types cross-ref, install.md additive example) were folded. DevX's shell-quoting hint was folded. The only remaining deferrals are cap-blocked single-line edits (sub-microsecond perf, a mutation comment) and an out-of-scope feature (--remove-skill). None of these represent user-facing risk.

Strategically, this PR closes a reliability gap that would erode trust in --skill as an incremental-deploy primitive -- the exact workflow enterprise adopters need when governing large agent bundles. Shipping it now reinforces the 'no silent data loss' contract that differentiates APM from manual-copy workflows.

Aligned with: portable by manifest (additive subset persisted in apm.yml; manifest stays the single source of truth, portable across machines); secure by default (union sources from the manifest, not the lockfile, so no previously-removed skill silently re-deploys without explicit intent); pragmatic as npm (additive install --skill matches the npm install <pkg> mental model -- each invocation adds, never silently removes).

Growth signal. The oss-growth-hacker flagged a concrete reliability story: 'we fixed silent data loss in incremental skill deployment.' This is a credible trust-building narrative for the next patch release note and a strong signal for enterprise evaluators who need proof that APM treats deployed artifacts as durable. Worth a one-line callout in the CHANGELOG entry.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	2	Clean additive-union design; helper extraction is sound; SRP respected.
CLI Logging Expert	0	0	0	Output paths consistent; verbose breadcrumb well-formed and ASCII-clean. Ship clean.
DevX UX Expert	0	0	2	Additive semantics match the package-manager mental model; reset path is discoverable.
Supply Chain Security Expert	0	0	0	Union sources from the manifest, not the lockfile; skill_subset is inventory metadata, not a trust anchor. No findings.
OSS Growth Hacker	0	0	0	Reliability beat: install --skill is now truly additive -- a credible "we fixed silent data loss" story.
Doc Writer	0	1	2	Docs now state the additive contract clearly (round-2 folds applied).
Test Coverage Expert	0	0	2	Behavior well-covered; the two helper unit-test gaps were addressed this round (17 passing).
Performance Expert	0	0	1	No hot-path impact; union is O(n) over a tiny skill list.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 3 follow-ups

1. [DevX UX Expert] Open a follow-up issue for a --remove-skill X or --skill -X subtractive flag to complete the additive/subtractive model. -- Additive-only is sound today, but enterprise users managing many skills will eventually need explicit subtraction without a full --skill '*' reset.
2. [Performance Expert] Remove the redundant get_identity() recompute inside apply_cli_skill_pin once the install.py line cap is addressed. -- Sub-microsecond today, but redundant work signals copy-paste drift; the fix is one line once the cap allows.
3. [Python Architect] Add a call-site comment in install.py noting that apply_cli_skill_pin mutates both dep_ref and apm_yml_entries in place. -- Mutation of two arguments is a maintainability surprise; a single-line comment is cheap insurance against future bugs.

Architecture

classDiagram
    class package_resolution {
        +effective_deploy_skill_subset(from_cli, cli_subset, persisted) tuple|None
        +cli_skill_subset(skill_names) tuple|None
        +apply_cli_skill_pin(dep_ref, cli_subset, from_cli, deps, entries) None
        +normalize_and_merge_skill_subset(...) list
    }
    class template_deploy {
        +deploy(ctx, dep_ref) None
    }
    class lockfile_phase {
        +_attach_skill_subset_override(locked_dep, merged) None
    }
    class install_cmd {
        +install(...) None
    }
    install_cmd ..> package_resolution : cli_skill_subset / apply_cli_skill_pin
    template_deploy ..> package_resolution : effective_deploy_skill_subset
    lockfile_phase ..> package_resolution : effective_deploy_skill_subset

Loading

flowchart TD
    A[apm install bundle --skill B] --> B{cli_skill_subset}
    B -->|'*' or absent| C[None = deploy all]
    B -->|named| D[CLI subset tuple]
    D --> E[effective_deploy_skill_subset]
    F[apm.yml persisted subset] --> E
    E --> G[additive union: prior + new]
    G --> H[template deploy: write union to disk]
    G --> I[lockfile phase: persist union]
    C --> J[reset: deploy full bundle, apm.yml = plain entry]

Loading

sequenceDiagram
    participant U as User
    participant CLI as install.py
    participant PR as package_resolution
    participant T as template.py
    participant L as lockfile.py
    U->>CLI: apm install bundle --skill B
    CLI->>PR: cli_skill_subset(("B",))
    PR-->>CLI: ("B",)
    CLI->>PR: apply_cli_skill_pin(dep_ref, ...)
    PR-->>CLI: dep_ref.skill_subset = union(persisted, "B")
    CLI->>T: deploy(dep_ref)
    T->>PR: effective_deploy_skill_subset(...)
    PR-->>T: ("A","B") union
    T-->>U: A and B both on disk (A preserved)
    CLI->>L: persist union to lockfile

Loading

Recommendation

Merge as-is. CI is green, all 17 tests pass, every in-scope recommendation was folded or addressed this round, and no panelist raised a blocking finding. The three deferred items are cap-blocked single-line edits or out-of-scope features -- track as follow-up issues post-merge. The reliability fix is user-facing and time-sensitive; delaying for cosmetic debt would be wrong.

---

Full per-persona findings

Python Architect

• [nit] lockfile.py list(merged or []) readability at src/apm_cli/install/phases/lockfile.py
  Ambiguous truthiness guard. Folded this round: now list(merged) if merged else [] with a clarifying comment.
• [nit] apply_cli_skill_pin mutates two args (dep_ref + apm_yml_entries) in place at src/apm_cli/install/package_resolution.py
  Wants a side-effect note at the call site. Deferred: the clean fix adds a line to install.py, which is at the 2100-line CI cap.

CLI Logging Expert

No findings.

DevX UX Expert

• [nit] --skill '*' help text should hint shell-quoting at src/apm_cli/commands/install.py
  Folded this round (in-place, net-zero-line edit).
• [nit] A future --remove-skill X / --skill -X subtractive flag would complete the model.
  Deferred as an out-of-scope follow-up issue candidate.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings. Reliability story flagged for the release note (see Growth signal above).

Doc Writer

• [recommended] package-types.md missing additive cross-ref at docs/src/content/docs/reference/package-types.md
  Folded this round.
• [nit] install.md example should demonstrate additive-across-two-installs at docs/src/content/docs/reference/cli/install.md
  Folded this round.
• [nit] migration.md -- persona itself said leave as-is. No action.

Test Coverage Expert

• [nit] No dedicated unit test for apply_cli_skill_pin reset branch.
  Addressed this round: added reset-branch + no-op-branch unit tests.
• [nit] None for cli_skill_subset.
  Addressed this round: added absent / '*' / named / mixed-'*' unit tests.
  Proof (passed): tests/unit/install/test_skill_subset_deploy_parity.py -- 17 passed.

Performance Expert

• [nit] apply_cli_skill_pin recomputes get_identity() the caller already holds at src/apm_cli/install/package_resolution.py
  Deferred: sub-microsecond; the clean fix adds a line to the capped install.py.

Auth Expert -- inactive

no auth surface -- PR touches install deploy/lockfile parity only, no token/credential/host-resolution paths

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}