fix(update): re-check transitive dependencies' own semver ranges during apm update

[nadav-y]

Summary

This branch's diff contains three bugs in apm update's dependency resolution and rollback safety. Bug 1 was already fixed by #2050 and is only present here because this branch stacks on it. Bugs 2 and 3 are new in this PR.

Bug 1 (fixed in #2050): torn state on declined/aborted update. Resolving a semver range downloads the candidate version to disk before the user confirms the update plan. A declined confirmation, a non-interactive abort, or --dry-run used to leave apm_modules/ already advanced while apm.lock.yaml stayed on the old version.

Bug 2 (this PR): transitive dependencies never get re-checked. apm update was not picking up a new version of a transitive dependency, even when the intermediate package's manifest still allowed it:

org/pkg1 -> org/pkg2 (^1.0.0) -> org/pkg3 (^1.0.0)

Publishing a new org/pkg3 matching ^1.0.0 was silently ignored by apm update run from org/pkg1, even though org/pkg2's manifest hadn't changed. Root cause: APMDependencyResolver._try_load_dependency_package only calls download_callback (which re-resolves a semver range against the remote) when the install path doesn't already exist:

if not install_path.exists():
    ...call download_callback...

#2050 forced this recheck for direct dependencies only (a pre-purge pass that ran before BFS resolution started, since transitive deps aren't known until their parent's manifest is read). Once org/pkg2 exists locally, org/pkg3's own range is never re-evaluated -- the walk sees its path already exists and stops.

Bug 3 (this PR, caught in Copilot review): a failed backup silently allowed an unprotected overwrite. backup_before_overwrite swallowed every exception during staging and returned False. If staging failed (e.g. a rename error) on an existing install path, the caller proceeded to overwrite it anyway with no rollback point. A later declined/aborted update would then see that dep as downloaded but with no backup entry -- indistinguishable from a fresh add -- and delete it outright, permanently losing the original content.

Fix

• apm_resolver.py: Added APMDependencyResolver._should_force_recheck(dep_ref), gated on a new update_refs flag. Widens the existing-path check to if not install_path.exists() or self._should_force_recheck(dep_ref):, so download_callback runs for any non-local, non-artifactory-proxied, semver-ranged dependency at any depth. (fixes Bug 2)
• resolve.py: Removed the direct-only pre-purge pass (no longer needed); threads update_refs into the resolver.
• update_backup.py: Moved backup staging inline into download_callback, called immediately before a dependency's install path is overwritten, for direct and transitive deps alike -- sidesteps the chicken-and-egg problem of not knowing transitive deps ahead of time. ctx.update_backups now stores dep_key -> (dep_ref, backup_path) tuples so a transitive dep's backup survives even if it never reaches ctx.all_apm_deps/ctx.deps_to_install. (fixes Bug 2's backup coverage)
• update_backup.py: backup_before_overwrite no longer suppresses staging exceptions -- it now raises when a backup is actually required, so the caller never proceeds to overwrite without one. The existing resolve-phase exception handler in pipeline.py restores any backups already staged for other deps this run before re-raising, so the whole update aborts cleanly instead of silently losing content. (fixes Bug 3)
• context.py / pipeline.py: Comment updates only, no functional change.

A regression surfaced during development: widening only the resolver gate (without widening backup coverage) caused restore_update_backups to treat newly-reachable transitive deps as fresh adds with no backup, deleting them outright on decline. Caught via live repro before landing the inline-backup fix.

Test plan

• New unit tests in test_apm_resolver_edge_cases.py (TestShouldForceRecheck, TestTryLoadDependencyPackageForceRecheck) covering the recheck predicate and gate across semver/literal ref kinds, local deps, artifactory-proxied deps, and update_refs on/off.
• Rewrote test_update_backup.py for the inline, tuple-based API, including a transitive-dep-not-in-all_apm_deps regression test and a staging-failure test asserting it raises with the original content untouched.
• Full unit suite: 18177 passed, 7 pre-existing unrelated failures (mcp_integrator/global_mcp_scope/lifecycle_executor), 0 new failures.
• Live repro against a real registry:
  • 3-level chain, all ^1.0.0: update reaches depth 3 across successive publishes.
  • Out-of-range major bump (2.0.0 against ^1.0.0): correctly ignored.
  • Exact-pin then semver (pkg1 pins pkg2 exactly, pkg2 ranges pkg3): innermost dep still updates through the exact-pinned intermediate.
  • 4-level chain: update reaches depth 4.
  • Non-interactive decline with a pending update: lockfile hash unchanged, content untouched, backup dir cleaned up.
  • Happy path (-y) and apm install --update (no confirm gate): both apply cleanly.
  • Combined with fix(update): stop declined/dry-run apm update from mutating apm_modules/ #2050 and fix(skills): key ownership tracking on owner/repo, not the leaf directory name #2052 on one branch, plus a live cross-package skill-collision scenario, to rule out interaction bugs between the three fixes.

Branch note

Stacked on #2050 (fix/update-plan-gate-torn-state) -- same update_backup.py mechanism, so the diff includes #2050's commits until it merges. Please review/merge #2050 first; commits unique to this PR start after 2683e8ea.

[Copilot]

Pull request overview

This PR fixes apm update so that semver-ranged dependencies are re-evaluated against the remote at any depth (including transitives), and extends the staged-backup/rollback mechanism so update-plan confirmation can safely decline/abort without leaving apm_modules/ ahead of apm.lock.yaml.

Changes:

• Widen dependency resolution so existing install paths can still invoke download_callback when update_refs=True and the dep is a non-local, non-proxied semver ref.
• Move update rollback staging to an inline backup_before_overwrite() call inside download_callback, and reconcile staged backups at the plan-confirmation gate (plus early restore on resolve-phase exceptions).
• Add/refresh unit tests for the new recheck predicate and the inline backup/restore behavior; add changelog entries for #2050 and this fix.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/apm_cli/deps/apm_resolver.py	Adds update_refs + _should_force_recheck() and widens the existing-path gate so transitive semver deps can be rechecked.
src/apm_cli/install/phases/resolve.py	Removes direct-only pre-purge and stages backups inline before overwrites; threads update_refs into the resolver.
src/apm_cli/install/phases/update_backup.py	Introduces the inline backup + restore reconciliation helpers for update-plan gating.
src/apm_cli/install/pipeline.py	Plumbs plan_callback into context and ensures backups are reconciled on plan decision (and restored on resolve exceptions when staged).
src/apm_cli/install/context.py	Documents and adds plan_callback and the new update_backups shape on InstallContext.
tests/unit/deps/test_apm_resolver_edge_cases.py	Adds coverage for _should_force_recheck() and for callback invocation on existing paths when updating.
tests/unit/install/phases/test_update_backup.py	New unit tests for sanitization + inline staging + restore behavior (including a transitive-only restore case).
tests/unit/test_install_pipeline_orchestration.py	Adds coverage ensuring resolve-phase failures trigger restore when backups were staged.
CHANGELOG.md	Adds Unreleased fixes entries for #2050 and #2053.

[sergio-sisternes-epam]

APM Review Panel: ship_with_followups

Depth-correct transitive semver re-resolution is structurally sound; ship with follow-up issues for path containment, decline-path test gap, and predicate dedup.

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

This PR closes a real correctness gap in apm update: transitive dependencies with semver ranges were silently skipped during re-resolution because download_callback only fired when an install path didn't already exist. The fix is architecturally clean -- a new backup/restore module with fail-closed semantics, a predicate gate that short-circuits to zero cost on apm install, and 577 lines of new unit tests. All nine panelists agree the change is directionally correct and none raised a blocking finding. The strongest consensus signal is the duplicated predicate between _should_force_recheck and _force_semver_resolve, flagged independently by python-architect, devx-ux, and performance. The 'kept in sync deliberately' comment is honest but not mechanical enforcement; extracting a shared function is low-risk, high-payoff, and should land before the next contributor touches either site.

The supply-chain finding on path containment deserves strategic weight beyond its technical scope. APM's security model routes all path deletions through ensure_path_within / safe_rmtree chokepoints -- ad-hoc regex sanitization in a new module is a precedent violation, not just a style issue. The fix is zero-behavioral-change (one import, one call), and leaving it out creates a citable gap if APM's supply-chain posture is ever audited. I side with the supply-chain expert: this should be a fast follow-up issue, not deferred.

The test-coverage expert's two missing-evidence findings carry real weight. The decline-path restore assertion (outcome: missing on a secure-by-default surface) is the most important gap: the entire user promise of 'say no and nothing changes' has no automated guardrail proving restore_update_backups is called with keep_new=False. This is a regression trap waiting to happen. The integration-tier gap is lower urgency -- the cross-module contract is covered by unit mocks, and the backup/restore I/O is simple enough that unit coverage is defensible for now -- but it should be filed as a follow-up to close before the next release.

Dissent. No inter-panelist disagreements on severity classification. Three panelists (python-architect, devx-ux, performance) independently flagged the duplicated predicate; python-architect classified it recommended while devx-ux and performance classified it nit. CEO sides with python-architect's recommended classification: two copies of a gate predicate that must agree on a correctness-critical path is a maintenance hazard, not a style preference.

Aligned with: Pragmatic-as-npm: apm update now reaches full transitive depth for semver deps, matching the depth-correctness users expect from any mature package manager. Zero regression on apm install. | Secure-by-default: Fail-closed on backup-staging failure is correct; path containment gap is a fast follow-up. | OSS-community-driven: 577 lines of new tests from a contributor signal engineering rigor that builds long-term trust.

Growth signal. This PR is a trust-builder worth surfacing at release time. Frame as 'apm update is now depth-correct for semver across transitive dependencies' -- it positions APM alongside mature package managers and closes a silent-failure gap that would become a FAQ-generator as transitive dep trees grow.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	Well-decomposed backup/restore module with correct fail-closed semantics; duplicated predicate is the main structural concern.
CLI Logging Expert	0	2	1	Backup/restore lifecycle is completely silent -- no verbose_detail on stage, no user-facing message on restore after decline/abort.
DevX UX Expert	0	0	2	Transitive semver re-resolution fix is UX-sound: update reaches full depth, declined plans restore cleanly, install is unaffected, no command-surface changes.
Supply Chain Security Expert	0	1	1	Backup module uses ad-hoc sanitization instead of the sanctioned path_security guards; suppress(Exception) in restore silences restore failures.
OSS Growth Hacker	0	0	3	High-trust correctness fix with excellent CHANGELOG prose; minor release-note framing opportunity missed but nothing blocks merge.
Auth Expert	0	0	0	No auth surface affected: auth_resolver still flows to resolver unchanged; new backup/restore module is auth-agnostic; wider download_callback reach reuses existing per-dep token resolution.
Doc Writer	0	2	2	Bug 3 (fail-closed staging failure) has no CHANGELOG entry; update.md omits transitive semver recheck behavior; two nits on mechanism wording and attribution style.
Test Coverage Expert	0	2	1	Strong unit coverage on predicate + backup/restore module; pipeline decline path missing restore assertion; no integration-tier test for transitive semver re-resolution flow.
Performance Expert	0	1	4	Correct approach; added cost is O(unique_repos) network RTTs for semver transitive deps during apm update, zero regression on apm install; backup I/O is ~1 rename per dep.

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

Top 5 follow-ups

1. [Test Coverage Expert] Add unit assertion that decline path calls restore_update_backups(ctx, keep_new=False) -- Missing automated guardrail on a secure-by-default surface: the user promise 'decline an update and nothing changes' has no test proving restore is invoked.
2. [Supply Chain Security Expert] Route backup_path through ensure_path_within and use safe_rmtree instead of _rrm in restore -- APM's security model requires all path deletions to flow through sanctioned chokepoints. Zero-behavioral-change fix.
3. [Python Architect] Extract shared predicate from _should_force_recheck and _force_semver_resolve -- Three panelists independently flagged the duplicated gate condition. A shared function prevents silent drift on a correctness-critical predicate.
4. [Doc Writer] Add CHANGELOG entry for Bug 3 (fail-closed on backup-staging failure) and update update.md to mention transitive semver re-evaluation -- Bug 3 has no CHANGELOG bullet; docs page scopes re-resolution to direct deps only, which is now inaccurate.
5. [CLI Logging Expert] Emit verbose_detail on backup staging and user-facing summary on restore after decline/abort -- Replacement for the removed _purge log; restore after decline should confirm rollback happened.

Architecture

classDiagram
    direction LR
    class APMDependencyResolver {
        <<Strategy>>
        +_update_refs bool
        +_download_callback DownloadCallback
        +_should_force_recheck(dep_ref) bool
        +_try_load_dependency_package(dep_ref, parent_chain, parent_pkg) APMPackage
    }
    class InstallContext {
        <<ValueObject>>
        +plan_callback Any
        +update_backups dict
        +callback_downloaded dict
        +apm_dir Path
        +apm_modules_dir Path
    }
    class DependencyReference {
        <<ValueObject>>
        +ref_kind str
        +is_local bool
        +get_unique_key() str
        +get_install_path(modules_dir) Path
    }
    class backup_before_overwrite {
        <<Pure Function>>
        +__call__(ctx, dep_ref, install_path) bool
    }
    class restore_update_backups {
        <<Pure Function>>
        +__call__(ctx, keep_new) None
    }
    class download_callback {
        <<Closure>>
        +_force_semver_resolve bool
    }
    class run_install_pipeline {
        <<IOBoundary>>
    }
    APMDependencyResolver ..> DependencyReference : checks ref_kind
    APMDependencyResolver ..> download_callback : invokes when gate opens
    download_callback ..> backup_before_overwrite : calls before overwrite
    download_callback ..> InstallContext : writes callback_downloaded
    backup_before_overwrite ..> InstallContext : writes update_backups
    backup_before_overwrite ..> DependencyReference : reads unique key
    run_install_pipeline ..> restore_update_backups : calls on resolve failure and plan gate
    restore_update_backups ..> InstallContext : reads update_backups and callback_downloaded
    classDef touched fill:#fff3b0,stroke:#d47600
    class APMDependencyResolver:::touched
    class backup_before_overwrite:::touched
    class restore_update_backups:::touched
    class InstallContext:::touched
    class download_callback:::touched

Loading

flowchart TD
    A["apm update CLI"] --> B["run_install_pipeline\npipeline.py"]
    B --> C["_run_phase resolve\npipeline.py"]
    C --> D["APMDependencyResolver.resolve\napm_resolver.py"]
    D --> E{"install_path.exists()\nAND NOT _should_force_recheck?"}
    E -->|"Yes: skip"| F["Load cached APMPackage\nfrom disk"]
    E -->|"No: gate open"| G["download_callback\nresolve.py"]
    G --> H{"_force_semver_resolve?"}
    H -->|"Yes"| I["[NET] git ls-remote"]
    H -->|"No"| J["Registry or git path"]
    I --> K["[FS] backup_before_overwrite\nupdate_backup.py"]
    J --> K
    K --> L{"plan_callback set\nAND path exists?"}
    L -->|"Yes"| M["[FS] install_path.rename backup_path"]
    L -->|"No"| N["No-op"]
    M --> O["ctx.update_backups[dep_key] = entry"]
    N --> P["[NET] download/clone to install_path"]
    O --> P
    P --> Q["Return to BFS"]
    C -->|"Exception"| R["[FS] restore_update_backups\nkeep_new=False"]
    R --> S["Re-raise"]
    B --> T["build_update_plan"]
    T --> U["plan_callback confirm gate"]
    U -->|"Declined / abort"| V["[FS] restore_update_backups\nkeep_new=False"]
    U -->|"Confirmed"| W["[FS] restore_update_backups\nkeep_new=True"]
    V --> X["Return empty InstallResult"]
    W --> Y["Continue to integrate phase"]

Loading

Recommendation

This is a correctness fix for a real silent-failure gap in apm update, architecturally sound, with strong test coverage from a contributor. No panelist raised a blocking finding. The five follow-ups (decline-path test assertion, path containment, predicate dedup, CHANGELOG/docs gap, verbose logging) should be filed as issues and resolved before the next release, but none block merge of this PR.

---

Full per-persona findings

Python Architect

• [recommended] _should_force_recheck duplicates _force_semver_resolve -- extract shared predicate at src/apm_cli/deps/apm_resolver.py:886
  APMDependencyResolver._should_force_recheck and download_callback's _force_semver_resolve encode the same boolean condition with a 'kept in sync deliberately' comment. Two copies of a gate predicate that must agree is a maintenance hazard. Extract is_semver_recheck_eligible(dep_ref, update_refs) into a shared location.
  Suggested: Extract to deps/semver_utils.py or a DependencyReference property.

• [nit] backup_before_overwrite uses getattr defensively on a typed dataclass at src/apm_cli/install/phases/update_backup.py:97
  getattr guards are for loosely-mocked ctx in tests but undocumented in the function. A comment prevents future contributors from removing these.

• [nit] plan_callback typed as Any at src/apm_cli/install/context.py:82
  Known signature (plan: UpdatePlan) -> bool warrants Callable[[UpdatePlan], bool] | None.

CLI Logging Expert

• [recommended] backup_before_overwrite is completely silent at src/apm_cli/install/phases/update_backup.py:67
  Removed _purge_cached_semver_paths_for_update had a verbose_detail line. New backup emits nothing. Function returns True when staged so the caller can log -- but no caller uses the return value.
  Suggested: if backup_before_overwrite(ctx, dep_ref, install_path): logger.verbose_detail(f'[*] --update: staged rollback backup for {dep_ref.get_unique_key()}')

• [recommended] restore_update_backups emits no user-facing output when rolling back a declined/aborted update at src/apm_cli/install/phases/update_backup.py:129
  User declined the update but sees no confirmation rollback happened. Consider returning count of restored deps and emitting '[i] Restored N package(s) to pre-update state'.

• [nit] backup_before_overwrite docstring promises 'so the caller can log accordingly' but no caller uses the return value at src/apm_cli/install/phases/update_backup.py:102

DevX UX Expert

• [nit] No user-visible diagnostic when backup staging raises at src/apm_cli/install/phases/update_backup.py:114
  Raw OSError propagates without an actionable message. Consider wrapping with catch-and-reraise adding a one-line human hint.

• [nit] Duplicated predicate between _should_force_recheck and _force_semver_resolve at src/apm_cli/deps/apm_resolver.py:886

Supply Chain Security Expert

• [recommended] backup_path is not validated with ensure_path_within; deletions use _rrm instead of safe_rmtree at src/apm_cli/install/phases/update_backup.py:113
  Codebase security model requires path containment through ensure_path_within / safe_rmtree. Ad-hoc regex sanitization is a documented bug pattern. Zero-behavioral-change fix.
  Suggested: from apm_cli.utils.path_security import ensure_path_within; ensure_path_within(backup_path, backup_root) after computing backup_path. Use safe_rmtree in restore_update_backups.

• [nit] suppress(Exception) in restore_update_backups silences restore failures at src/apm_cli/install/phases/update_backup.py:178
  If restore fails, user silently ends up with missing package while lockfile references old version. Log a warning on suppressed exceptions.

OSS Growth Hacker

• [nit] CHANGELOG entry is maintainer-facing, not user-facing at CHANGELOG.md
  'download_callback only ran for a dependency whose install path didn't already exist' is implementation detail. User-facing rewrite: 'apm update now picks up newly published versions of transitive dependencies constrained by semver ranges (e.g. ^1.0.0)'.

• [nit] Two related entries (fix(update): stop declined/dry-run apm update from mutating apm_modules/ #2050 + fix(update): re-check transitive dependencies' own semver ranges during apm update #2053) could share a single story beat for stronger release impact.

• [nit] Story angle worth surfacing at release time: 'apm update now re-evaluates semver ranges at every depth -- not just direct deps.'

Auth Expert

No findings.

Doc Writer

• [recommended] Bug 3 (fail-closed on backup-staging failure) is absent from CHANGELOG at CHANGELOG.md
  Commit 813d1fb introduces a distinct user-visible safety guarantee with no CHANGELOG bullet.
  Suggested: Add: 'apm update now fails closed when staging a rollback point fails before overwriting an existing dependency: a rename or mkdir error during backup staging aborts the whole update cleanly instead of silently losing original content. (by @nadav-y) (fix(update): re-check transitive dependencies' own semver ranges during apm update #2053)'

• [recommended] docs/reference/cli/update.md does not mention transitive semver ranges are now re-evaluated at docs/src/content/docs/reference/cli/update.md:91
  Docs scopes re-resolution to direct deps only. Users with deep dep trees may see more packages updated than before.

• [nit] fix(update): stop declined/dry-run apm update from mutating apm_modules/ #2050 CHANGELOG entry says 'staged and rolled back' -- mechanism is backup-and-restore, not download staging.

• [nit] Attribution format deviates from predominant 0.24.0 style: use '(by @nadav-y, fix(update): re-check transitive dependencies' own semver ranges during apm update #2053)' not '(by @nadav-y) (fix(update): re-check transitive dependencies' own semver ranges during apm update #2053)'.

Test Coverage Expert

• [recommended] Plan callback decline path does not assert restore_update_backups is called with keep_new=False at tests/unit/test_install_pipeline_orchestration.py:204
  test_plan_callback_false_returns_early has no guardrail on the critical safety net for 'say no to update'.
  Proof (missing): tests/unit/test_install_pipeline_orchestration.py::TestRunInstallPipelinePlanCallback::test_plan_callback_false_returns_early_asserts_restore -- proves: A declined apm update restores backed-up install paths [devx, secure-by-default]

• [recommended] No integration-tier test for end-to-end transitive semver re-resolution backup/restore cycle at tests/integration/test_update_e2e.py
  Cross-module contract covered only by unit mocks. Install pipeline floor tier is integration-with-fixtures.
  Proof (missing): tests/integration/test_update_e2e.py::TestUpdateE2E::test_update_dry_run_transitive_semver_restores_modules [devx, secure-by-default]

• [nit] test_plan_callback_true_continues does not assert restore_update_backups(ctx, keep_new=True) is called at tests/unit/test_install_pipeline_orchestration.py:244

Performance Expert

• [recommended] Added network cost is O(unique_repos) RTTs via ref_resolver_cache sharing -- acceptable for explicit apm update.
  get_shared_ref_resolver reuses one RefResolver per (host, token) pair. For 5-15 transitive deps across 3-4 repos: 0-4 extra ls-remote calls. Acceptable.

• [nit] _should_force_recheck is zero-cost on apm install (update_refs=False short-circuits).

• [nit] Duplicate predicate is sync-maintenance risk, not a perf bug.

• [nit] backup_before_overwrite adds ~3ms I/O for 30 transitive deps -- negligible.

• [nit] Dedup gate prevents O(n^2) re-checks in diamond deps. Correct.

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

[sergio-sisternes-epam]

Panel review complete -- this PR is approved in principle but is stacked on #2050 which needs two fixes first (failing tests + suppress(Exception) narrowing). Will approve and queue for merge once #2050 lands.