fix(update): preserve deployment targets during partial updates - #2924
Daniel Meppiel (danielmeppiel) merged 10 commits into
Conversation
There was a problem hiding this comment.
🟢 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.
|
@microsoft-github-policy-service agree |
PR triage recommendationready-for-review Recommendation only, not merge or scope approval. A responsible human maintainer must approve. Labels, automated advice, and silence are not approval. Thank you for this pull request. It is linked to #2902 ( This is not merge approval and does not start a review or merge worker. CODEOWNERS already requested review from Daniel Meppiel (@danielmeppiel) and Sergio Sisternes (@sergio-sisternes-epam). This comment does not add, remove, or change review requests. Context on #2902: an earlier bot Suggested next step: wait for a human maintainer to review the accepted scope. Do not treat this comment or |
Sergio Sisternes (sergio-sisternes-epam)
added
triage/recommended
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Canonical merge is sound; one now-unreachable condition can be simplified. |
| DevX UX Expert | 0 | 0 | 1 | User-facing behavior and docs align; wording preference only. |
| Supply Chain Security | 0 | 0 | 0 | Ownership concern withdrawn after checking the existing collision contract. |
| Doc Writer | 0 | 0 | 0 | Docs describe preservation, not historical repair. |
| Test Coverage | 0 | 0 | 0 | Changed-commit integration regressions and canonical-state unit tests cover the fix proportionately. |
| Performance Expert | 0 | 0 | 0 | Redundant early merge removed; no introduced performance regression identified. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 1 follow-ups
- [Python Architect] Optionally remove
self.ctx.only_packages orfromsrc/apm_cli/install/phases/lockfile.py:416and adjust its nearby partial-install comment. The new early return already handles partial operations, so that disjunct is unreachable. This is housekeeping, not a correctness concern.
Architecture
classDiagram
direction LR
class LockfileBuilder {
+_attach_deployed_files(lockfile)
+_maybe_merge_partial(lockfile, path, LF)
}
class DeploymentReconciler {
+reconcile_package_claims()
+reconcile(previous, materializations, intent)
}
class DeploymentLedgerCodec {
<<Codec>>
+from_lockfile(lockfile)
+merge_dependencies(lockfile, updated)
+apply_to_lockfile(ledger, lockfile)
}
class DeploymentRecord {
+owners tuple
+active_owner str
}
class MaterializationResult {
+owners frozenset
+status MaterializationStatus
}
LockfileBuilder ..> DeploymentReconciler : current claims
LockfileBuilder ..> DeploymentLedgerCodec : partial merge
DeploymentReconciler ..> DeploymentRecord : produces
DeploymentReconciler ..> MaterializationResult : consumes
note for DeploymentReconciler "Current successful proofs determine refreshed ownership"
flowchart TD
A["_attach_deployed_files"] --> B["Reconcile current package claims"]
B --> C["Build current canonical records"]
C --> D["_maybe_merge_partial"]
D --> E["DeploymentLedgerCodec.merge_dependencies"]
E --> F["Capture current ledger"]
E --> G["Reconcile prior ledger excluding processed owners"]
F --> H["Update dependency metadata"]
G --> H
H --> I["Overlay current records onto retained records"]
I --> J["apply_to_lockfile once"]
J --> K["_write_if_changed: apm.lock.yaml"]
Recommendation
Ship now from the technical-review perspective. The only suggested code cleanup is cosmetic; the documented no-follow-up-install behavior is supported by the regression scenarios. Daniel and Sergio remain the requested reviewers. Scope and merge decisions remain human decisions; this advisory changes neither.
Full per-persona findings
Python Architect
- [nit] Unreachable partial-operation condition at
src/apm_cli/install/phases/lockfile.py:416.
_merge_existingnow returns early whenself.ctx.only_packagesis truthy. Its laterself.ctx.only_packages or dep_key in self.ctx.intended_dep_keyscondition therefore has an unreachable left-hand branch.
Suggested: Keep onlydep_key in self.ctx.intended_dep_keysand update the nearby comment accordingly.
DevX UX Expert
- [nit] The "no follow-up install needed" sentence describes the absence of the old bug rather than only the positive behavior.
The preceding target-preservation sentence already states the contract. Removing the second sentence is an optional tone preference, not a behavior concern.
Synthesis: Retain it if useful to readers arriving from [BUG] apm update writes target: legacy instead of copilot in v0.29.1 (regression from #2795 fix) #2902 and its documented workaround.
Supply Chain Security
No remaining findings. The initial owner-union suggestion was withdrawn after checking canonical current-proof ownership and the existing last-writer collision tests.
Doc Writer
No findings.
Test Coverage
No findings.
Performance Expert
No findings. No latency measurements or speedup claims are asserted.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.
e3e989c
into
microsoft:main
Description
Updating a selected dependency to a new commit can rewrite
copilotdeployment records aslegacy, 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 throughDeploymentLedgerCodecinstead: 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
Testing
Validated on macOS with Python 3.12.14, based on
d26ccdaf2fbcb29775abc94b7413790b3e32b600:{'copilot', 'legacy'} != {'copilot'}.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 missinggh, 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.