Fix --skill to merge additively with existing skills in apm.yml by sergio-sisternes-epam · Pull Request #1786 · microsoft/apm

sergio-sisternes-epam · 2026-06-15T16:23:00Z

Description

Running apm install repo --skill qa on a package that already has skills: [grill-me] in
apm.yml was replacing the list with [qa] instead of merging to [grill-me, qa]. This made
it impossible to incrementally build a skill subset across multiple install invocations.

The fix reads the existing skills: list from the manifest before applying the CLI value and
produces a deduplicated, sorted union -- matching the existing conventions in parse_from_dict
(sort on parse) and to_apm_yml_entry (sort on emit). The --skill '*' reset-to-all behaviour
is unchanged.

Fixes: #1771

Type of change

Bug fix
New feature
Documentation
Maintenance / refactor

Testing

Tested locally
All existing tests pass
Added tests for new functionality (if applicable)

Spec conformance (OpenAPM v0.1)

N/A -- this PR does not change OpenAPM-observable behaviour.

When running `apm install repo --skill qa` on a package that already has `skills: [grill-me]` in apm.yml, the CLI now merges the new name into the existing list (producing `skills: [grill-me, qa]`) instead of replacing it. - Extract `normalize_and_merge_skill_subset()` helper into `package_resolution.py` to keep install.py within its line budget. - Add `get_existing_skill_subset()` to read persisted skills: from current_deps by identity. - Update test expectations: skill_subset is now always sorted (matches the existing `to_apm_yml_entry()` and `parse_from_dict()` contracts). - Add integration-level tests proving repeated --skill invocations are additive and that no duplicates are introduced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Fixes apm install ... --skill so it merges additively with any already-persisted skills: list in apm.yml (instead of replacing it), enabling incremental skill subset building across multiple installs.

Changes:

Add get_existing_skill_subset() + normalize_and_merge_skill_subset() helpers to read/merge persisted skills: for a dependency identity.
Update install resolution to set dep_ref.skill_subset via the new merge helper when --skill is provided.
Add/adjust unit tests covering persisted-skill readback and additive merge behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`src/apm_cli/install/package_resolution.py`	Adds helpers to read existing `skills:` from the manifest and merge them with CLI `--skill`.
`src/apm_cli/commands/install.py`	Uses the new helper to make `--skill` additive for existing manifest entries.
`tests/unit/test_skill_subset_persistence.py`	Adds tests for reading existing persisted skills and validating additive merges in `_resolve_package_references`.
`tests/unit/commands/test_install_skill_subset.py`	Updates expected normalized ordering to match the now-sorted behavior.

…ive-merge

- Remove the intermediate normalized list that was never used in the return value; build only the seen set and return sorted(seen). - Update test docstring to say 'result is sorted' instead of 'preserving order' since the behaviour is now sorted output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Resolve strangler-fig conflicts: keep HEAD's relocated structure and fold main's #1786 additive-skill delta into install/pkg_resolution.py via normalize_and_merge_skill_subset. Shed mcp_integrator.py back under the 800-line gate by extracting _is_vscode_available into the new integration/mcp_vscode.py sibling (re-exported for the existing import and patch seams); repoint MCP shutil/Path patch targets to mcp_vscode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

lirantal · 2026-06-29T12:57:55Z

Was this released already? doesn't seem so from the latest version I am using so want to double check...

CI Stage-1 tightened the file-length guardrail to 2100 lines (was 2450); main's install.py already sits at 2099, so the additive --skill fix pushed it to 2116 and tripped the guard. Move the skill-pin handling into package_resolution.py -- the module that exists to keep the command module small: - cli_skill_subset(skill_names): normalize raw --skill names to a subset (None for absent / '*'), lifting the inline block out of install(). - apply_cli_skill_pin(...): attach/merge the additive pin (#1771) or reset to the full bundle on --skill '*' (#1786), deriving identity/canonical from the reference so the call site stays compact. install.py drops from 2116 to 2100 (== cap, passes the > comparison). Behaviour unchanged: parity suite + full install/commands unit dirs green (2817 passed); ruff, R0801, auth-signal, and file-length guards all clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The deployment/lockfile parity fix lands under install/ (an OpenAPM critical path) but introduces no net-new wire-format behaviour: the lockfile skill_subset field format and semantics are unchanged, and PR #1786 established the additive --skill contract without a spec requirement, so no anchor/manifest row applies. Re-triggers the spec-conformance workflow to re-read the PR-body waiver. apm-spec-waiver: bug fix to apm-CLI --skill additive deploy parity; no net-new OpenAPM wire-format surface -- lockfile skill_subset field/semantics unchanged; #1786 established the additive --skill contract without a spec requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…llow-up) (#1955) * Fix: make --skill deployment additive, not just persistence (#1786 follow-up) 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 manifest and on-disk state diverged until the next bare install. Changes: - New shared helper `effective_deploy_skill_subset()` in package_resolution.py: deploy the UNION of the persisted apm.yml `skills:` and the current CLI `--skill` values; `--skill '*'` returns None (deploy all). - template.py: deploy via the helper instead of the raw CLI subset. - phases/lockfile.py: `_attach_skill_subset_override` now unions the CLI values with the persisted subset so the lockfile matches what was deployed. - install.py: `--skill '*'` now resets the apm.yml `skills:` pin back to the full bundle (the documented "reset to all skills" contract), so manifest and on-disk state agree after a reset. Adds regression tests covering the helper's union/reset semantics and an e2e suite (additive deploy-on-top, bare reinstall, --skill '*' reset, manifest reduction cleanup, uninstall removes the union). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fold review-panel follow-ups: additive --skill deploy parity Round-1 apm-review-panel returned ship_with_followups (zero blocking). Fold all five in-scope CEO-curated recommendations: - lockfile.py: pass ctx.skill_subset_from_cli to effective_deploy_skill_subset instead of hardcoded True (consistency with template.py; defensive against future non-CLI callers). [py-architect + supply-chain convergent] - template.py: hoist effective_skill_subset to a named local and emit a verbose breadcrumb naming retained (previously-pinned) skills when the additive union exceeds the CLI request. [cli-logging] - install.py: verbose breadcrumb on --skill '*' pin reset; expand --skill help text to document additive union + single-skill removal path. [cli-logging + devx-ux] - docs/install.md + apm-usage/commands.md: document --skill additive-across- installs union + removal path (doc-sync Rule 4). [doc-writer] - CHANGELOG: add Unreleased Fixed entry framed as user-promise restoration. [growth] Breadcrumbs are verbose-only (verbose_detail), ASCII-only; no change to default output. Parity suite 11 passed; full lint chain green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Extract --skill pin helpers to keep install.py under the 2100-line cap CI Stage-1 tightened the file-length guardrail to 2100 lines (was 2450); main's install.py already sits at 2099, so the additive --skill fix pushed it to 2116 and tripped the guard. Move the skill-pin handling into package_resolution.py -- the module that exists to keep the command module small: - cli_skill_subset(skill_names): normalize raw --skill names to a subset (None for absent / '*'), lifting the inline block out of install(). - apply_cli_skill_pin(...): attach/merge the additive pin (#1771) or reset to the full bundle on --skill '*' (#1786), deriving identity/canonical from the reference so the call site stays compact. install.py drops from 2116 to 2100 (== cap, passes the > comparison). Behaviour unchanged: parity suite + full install/commands unit dirs green (2817 passed); ruff, R0801, auth-signal, and file-length guards all clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore(spec): waive Mode B gate for additive --skill deploy bug fix The deployment/lockfile parity fix lands under install/ (an OpenAPM critical path) but introduces no net-new wire-format behaviour: the lockfile skill_subset field format and semantics are unchanged, and PR #1786 established the additive --skill contract without a spec requirement, so no anchor/manifest row applies. Re-triggers the spec-conformance workflow to re-read the PR-body waiver. apm-spec-waiver: bug fix to apm-CLI --skill additive deploy parity; no net-new OpenAPM wire-format surface -- lockfile skill_subset field/semantics unchanged; #1786 established the additive --skill contract without a spec requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(install): fold review-panel round-2 nits for --skill parity Add hermetic unit coverage for the two helpers the additive --skill deploy fix relies on, and tighten doc + lockfile clarity per the panel: - cli_skill_subset: absent / '*' / named / mixed-'*' normalization - apply_cli_skill_pin: '*' reset branch (clears pin, records full-bundle apm.yml entry) and bare-install no-op branch - docs: package-types.md additive cross-ref; install.md additive example - lockfile.py: explicit `list(merged) if merged else []` + guard comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

danielmeppiel · 2026-06-29T17:25:25Z

@lirantal thought it was fixed, but wasn't, just merged #1955

Copilot AI review requested due to automatic review settings June 15, 2026 16:23

sergio-sisternes-epam requested a review from danielmeppiel as a code owner June 15, 2026 16:23

Copilot started reviewing on behalf of sergio-sisternes-epam June 15, 2026 16:23 View session

Copilot AI reviewed Jun 15, 2026

View reviewed changes

Comment thread src/apm_cli/install/package_resolution.py

Comment thread tests/unit/commands/test_install_skill_subset.py

romivol mentioned this pull request Jun 15, 2026

Add --add flag to preserve existing skills during install #1771

Closed

sergio-sisternes-epam and others added 2 commits June 15, 2026 23:31

Merge branch 'main' into sergio-sisternes-epam/fix-skill-subset-addit…

1d0fa5c

…ive-merge

danielmeppiel approved these changes Jun 16, 2026

View reviewed changes

danielmeppiel added this pull request to the merge queue Jun 16, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 16, 2026

sergio-sisternes-epam added this pull request to the merge queue Jun 16, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 16, 2026

danielmeppiel added this pull request to the merge queue Jun 16, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 16, 2026

danielmeppiel added this pull request to the merge queue Jun 19, 2026

Merged via the queue into main with commit 94897b8 Jun 19, 2026
27 checks passed

danielmeppiel deleted the sergio-sisternes-epam/fix-skill-subset-additive-merge branch June 19, 2026 08:59

danielmeppiel mentioned this pull request Jun 25, 2026

Clarify or make additive --skill behavior when installing from existing skill bundles #1862

Closed

danielmeppiel mentioned this pull request Jun 29, 2026

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix --skill to merge additively with existing skills in apm.yml#1786

Fix --skill to merge additively with existing skills in apm.yml#1786
danielmeppiel merged 3 commits into
mainfrom
sergio-sisternes-epam/fix-skill-subset-additive-merge

sergio-sisternes-epam commented Jun 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lirantal commented Jun 29, 2026

Uh oh!

danielmeppiel commented Jun 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

sergio-sisternes-epam commented Jun 15, 2026

Description

Type of change

Testing

Spec conformance (OpenAPM v0.1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lirantal commented Jun 29, 2026

Uh oh!

danielmeppiel commented Jun 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants