Skip to content

fix(update): preserve deployment targets during partial updates - #2924

Open
Wanming08 wants to merge 9 commits into
microsoft:mainfrom
Wanming08:fix/update-deployment-target
Open

fix(update): preserve deployment targets during partial updates#2924
Wanming08 wants to merge 9 commits into
microsoft:mainfrom
Wanming08:fix/update-deployment-target

Conversation

@Wanming08

Copy link
Copy Markdown

Description

Updating a selected dependency to a new commit can rewrite copilot deployment records as legacy, including records for untouched skills under .agents/skills/. A subsequent install restores them. The regression reproduces on current main when the selected package has actually changed; a full update or a no-op partial update does not reproduce it.

The partial lockfile merge calls add_dependency, which invalidates the canonical deployment ledger. Rebuilding from flat paths loses target provenance for shared directories. Merge through DeploymentLedgerCodec instead: reconcile out the processed owners, retain untouched canonical records, overlay the current deployment records, and apply the merged ledger once. Skip the redundant early partial metadata merge so the processed owner set stays accurate.

Adds real CLI regressions for updating either a skill or an instruction, plus coverage for added files, unchanged dependency commits, surviving shared owners, canonical locators, and serialization. The existing architecture guard now requires the canonical merge call. Command documentation and the shipped usage guide describe the restored behavior.

Fixes #2902

Type of change

  • Bug fix
  • Documentation
  • New feature
  • Maintenance / refactor

Testing

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

Validated on macOS with Python 3.12.14, based on d26ccdaf2fbcb29775abc94b7413790b3e32b600:

  • Virtual package lifecycle matrix: 8 passed. Both new parameterized cases fail against the unmodified implementation with {'copilot', 'legacy'} != {'copilot'}.
  • Deployment state, lockfile union, and lockfile tests: 163 passed.
  • Spec conformance: 202 passed, 2 skipped. Test quality: 63 passed.
  • Ruff lint/format, duplicate-code lint, auth lint, architecture boundaries, assertion-quality and exact-duplicate ratchets, and spec orphan checks passed.

The complete unit/console run, excluding two environment failures already reproduced on clean main, finished with 22,270 passed, 28 failed, 42 skipped, 21 xfailed. All 28 remaining failures also reproduce on clean d26ccda. The 30 baseline failures involve missing gh, DNS-dependent HTTP/SSRF cases, and local HTTP/WebSocket tests affected by this machine's proxy configuration. No tests or production networking safeguards were changed to hide these failures.

Spec conformance (OpenAPM v0.1)

This restores existing declared-target provenance during partial updates; it introduces no schema or normative requirement change. The existing conformance suite passes. Mode B reports 8 substantive added lines in its monitored paths and passes without a waiver; the orphan check confirms all 122 requirements remain aligned.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The partial-update merge now uses the canonical deployment-ledger merge path with targeted unit/integration regressions and accompanying documentation/changelog updates.

Pull request overview

Fixes a regression where apm update (when run as a partial update against selected packages) could rewrite deployment target records in apm.lock.yaml to legacy by invalidating and rebuilding the deployment ledger without preserving target provenance; aligns partial-update merging with the canonical DeploymentLedgerCodec path so untouched records retain their original target information.

Changes:

  • Route partial lockfile merges through DeploymentLedgerCodec.merge_dependencies() to preserve canonical deployment ledger records (including target provenance) while updating only the selected dependencies.
  • Add unit + integration regressions ensuring partial updates preserve deployment targets and correctly retain untouched deployment records and locators.
  • Document the restored behavior and add a changelog entry.
File summaries
File Description
tests/unit/core/test_deployment_state.py Adds a focused unit test covering dependency merge behavior for canonical locators, surviving owners, MCP target rows, and round-trip serialization.
tests/integration/test_virtual_package_lifecycle_matrix.py Adds an integration regression verifying partial updates preserve target: copilot for both updated and untouched deployments (skill + instruction cases).
src/apm_cli/install/phases/lockfile.py Skips early “existing lockfile” merge for partial operations and performs the partial merge via the canonical deployment-ledger merge helper.
src/apm_cli/core/deployment_ledger.py Introduces DeploymentLedgerCodec.merge_dependencies() to merge updated deps without discarding canonical deployment locators/targets.
scripts/architecture_linter/checks/install_base_integrator_and_contraction.py Extends the architecture guardrail to require the canonical merge_dependencies call in the partial-merge path.
packages/apm-guide/.apm/skills/apm-usage/commands.md Updates the shipped usage guide to describe preserved deployment targets during partial updates.
docs/src/content/docs/reference/cli/update.md Updates CLI reference docs to reflect that partial updates preserve deployment targets without requiring a follow-up install.
CHANGELOG.md Adds an Unreleased “Fixed” entry describing the regression fix.
Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@Wanming08

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] apm update  writes  target: legacy  instead of  copilot  in v0.29.1 (regression from #2795 fix)

2 participants