fix(update): stop declined/dry-run apm update from mutating apm_modules/

[nadav-y]

Summary

apm update computes its plan by re-resolving semver ranges during the resolve phase -- for both git-source and registry-source dependencies, that re-resolution already downloads and writes the new version's content to apm_modules/, before the plan-confirmation gate ever runs. When the gate does not commit -- the user declines the prompt, the shell is non-interactive (no TTY, no --yes), or --dry-run is passed -- the new content stays on disk while apm.lock.yaml stays on the old version. The project is left in a torn state: apm_modules/ ahead of the lockfile, with no error surfaced.

Reproduced live against a real registry and a real GitHub repo:

Scenario	Before this PR	After this PR
apm update declined (n)	lockfile unchanged, files silently updated	both unchanged
apm update non-interactive, no --yes (exits 1)	lockfile unchanged, files silently updated	both unchanged
apm update --dry-run ("no changes applied")	lockfile unchanged, files silently updated	both unchanged
apm update -y / confirmed y	correctly updates both	unchanged (still correct)
apm outdated	already safe, read-only	unchanged
apm install --update (no confirm gate)	always applies, nothing to decline	unchanged

Fix

_purge_cached_semver_paths_for_update forces re-resolution by deleting a semver dep's cached install path so download_callback can't short-circuit on install_path.exists(). This PR changes it to move the path aside to a backup instead of deleting it -- but only when apm update's plan_callback is present (apm install --update has no decline path and keeps the original delete-and-redownload behavior byte-for-byte).

Once the plan-confirmation gate resolves, a try/finally in pipeline.py reconciles the outcome:

• Committed: discard the backups; the freshly-downloaded content is correct and stays.
• Not committed (declined / non-interactive abort / --dry-run): restore every backed-up dep's original content, and remove any freshly-downloaded dep that had no prior backup (a new add swept into the same resolve pass).

The finally runs even when the non-interactive path raises SystemExit, so the rollback can't be skipped by that abort.

The purge/restore logic moved into a new install/phases/update_backup.py (also keeps resolve.py under its 1000-LOC architecture-invariant budget, which my initial in-place patch pushed past).

Test plan

• New unit tests in tests/unit/install/phases/test_update_backup.py (13 tests) covering: outright-delete parity when no backup root is given, non-semver/local/artifactory deps skipped, backup-and-restore round trip, commit discards backup, decline restores original content, a freshly-added dep with no backup is removed on decline, a purged-but-never-redownloaded dep still restores even when the overall run commits, empty backup dir cleanup, multi-dep isolation.
• Full unit suite (pytest tests/unit/): 18155 passed, 8 pre-existing failures confirmed unrelated (verified identically failing on main before this change), 0 new failures.
• ruff check / ruff format --check: clean.
• Live manual repro against a real JFrog registry package (publish v1.0.0 + v1.0.1, pin lockfile to v1.0.0, semver range ^1.0.0): confirmed the bug before the fix, confirmed both the fix (declined/non-interactive/dry-run all leave state untouched) and the happy path (-y still updates correctly) after.
• Live manual repro against a real GitHub repo (microsoft/apm itself, tags v0.23.0 -> v0.23.1, semver range ^0.23.0): same result -- confirmed for git-sourced deps too, not registry-specific.

Spec conformance

This is a bug fix restoring behaviour the plan-confirmation gate already
promised (its own code comment: confirmation happens "before downloads
begin so a 'no' answer cancels cleanly without touching the cache") --
it does not introduce new normative behaviour, so no new spec anchor /
manifest row / Appendix C entry applies.

apm-spec-waiver: bug fix restoring existing update-confirm-gate no-side-effect contract

[Copilot]

Pull request overview

This PR fixes a torn-state bug in apm update where semver re-resolution during the resolve phase could mutate apm_modules/ even when the plan-confirmation gate does not commit (decline, non-interactive abort, or --dry-run). It stages existing installs for rollback and reconciles them after the plan gate.

Changes:

• Add an update backup/restore mechanism to stage existing semver installs during apm update and roll them back when the plan is not committed.
• Wire the plan-callback signal through InstallContext and reconcile backups around the plan-confirmation gate.
• Add unit tests covering backup/restore behavior and add a changelog entry for the fix.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tests/unit/install/phases/test_update_backup.py	Adds unit tests for the stage-and-restore mechanism and rollback edge cases.
src/apm_cli/install/pipeline.py	Passes plan_callback into InstallContext and reconciles staged backups in a try/finally around the plan gate.
src/apm_cli/install/phases/update_backup.py	Introduces purge-to-backup and restore/discard logic for apm update plan gating.
src/apm_cli/install/phases/resolve.py	Switches semver-cache purge to stage installs (when plan gate exists) instead of deleting outright.
src/apm_cli/install/context.py	Adds context fields for plan_callback and update_backups.
CHANGELOG.md	Adds an Unreleased “Fixed” entry describing the behavior change.

[nadav-y]

Consolidated into #2053, which now combines this fix with #2052 and the new transitive-semver-recheck fix on a single branch. Please review/merge #2053 instead of this one.

[sergio-sisternes-epam]

APM Review Panel: needs_rework

Two new pipeline orchestration tests FAIL on this commit (confirmed by pytest); suppress(Exception) on restore path can silently re-create the torn state this PR exists to fix. Core backup/restore mechanism is sound but the PR is not mergeable until the broken tests are fixed and the exception blanket is narrowed.

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

The strongest signal in this review is the test-coverage-expert's confirmed failing tests. TestRunInstallPipelineResolveFailureRestoresBackups (2 tests) fail with AttributeError because the mock patches apm_cli.install.phases.resolve instead of apm_cli.install.phases.resolve.run. This is not a pre-existing failure -- the test class is new in this PR. The resolve-failure->restore path therefore has zero passing coverage, which means the most critical code path this PR introduces (restore on failure) is unproven on this commit. This must be fixed before merge; it is not a follow-up.

Three panelists independently flagged suppress(Exception) in the restore/backup paths (python-architect, devx-ux-expert, supply-chain-security-expert). This is a genuine design defect, not a style nit: if restore_update_backups fails (permission error, disk full, EXDEV), the exception is swallowed and the user lands in exactly the torn state the PR exists to prevent -- apm_modules/ partially restored, lockfile unchanged, no error surfaced. The performance-expert's cross-filesystem finding is a concrete instance of this same flaw (Path.rename raises EXDEV, suppress eats it, backup is silently skipped). The fix is straightforward: narrow to OSError, log a warning, and consider surfacing a user-visible error on restore failure. This is blocking-tier work because it directly undermines the PR's stated contract.

The remaining findings -- silent rollback UX (devx-ux, cli-logging), backup directory not gitignored (supply-chain), and CHANGELOG wording (doc-writer, oss-growth) -- are real improvements but are safe to land as fast follow-ups. None of them introduce regression risk if deferred to a subsequent PR.

No substantive disagreement between panelists. The suppress(Exception) finding was raised independently by python-architect (recommended), devx-ux-expert (recommended), supply-chain-security-expert (nit), and performance-expert (recommended via EXDEV scenario). All agree on the direction (narrow the catch, add logging). I side with the recommended-tier classification: this is not a nit because it can silently reproduce the exact bug the PR fixes. The test-coverage-expert's finding carries outcome=failed evidence and outranks all opinion-only findings.

Aligned with: Portability by manifest (fix ensures manifest, lockfile, and apm_modules/ stay in sync); Secure by default (backup/restore improves integrity by default, but suppress(Exception) on restore path leaves a gap -- narrowing to OSError would make this fully clean); Pragmatic as npm (npm, cargo, bundler all guarantee decline is a no-op -- this PR brings apm update into alignment, the missing 'No changes made.' feedback is a small remaining gap).

Growth signal. 'Decline means nothing changed, always' is a trust-building message that resonates with enterprise evaluators and individual adopters alike. Recommend a punchy one-liner lead in the CHANGELOG and frame the release note around the atomicity guarantee. Contributor @nadav-y is a returning contributor fixing a real user-facing bug -- exactly the community signal worth amplifying.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	3	Well-structured extraction. Stage pattern is clean. suppress(Exception) in restore can re-create torn state.
CLI Logging Expert	0	1	2	restore_update_backups is completely silent -- no logging on restore, discard, or fresh-add removal.
DevX UX Expert	0	2	1	Decline path is correctly a no-op but gives zero user feedback; suppress(Exception) on restore risks torn state.
Supply Chain Security	0	1	1	PR improves lockfile integrity; backup dir not gitignored; suppress(Exception) overly broad for restore.
OSS Growth Hacker	0	0	2	Strong trust-building fix. CHANGELOG entry is release-notes-ready. Docs already describe the contract.
Auth Expert	--	--	--	Inactive: auth-neutral orchestration change only.
Doc Writer	0	1	3	CHANGELOG second sentence leaks internal mechanism detail; reference docs already correct.
Test Coverage Expert	0	2	1	16/16 update_backup unit tests pass; 2/2 new pipeline orchestration tests FAIL (confirmed by pytest).
Performance Expert	0	1	3	EXDEV cross-filesystem rename silently defeats backup gate; happy-path overhead negligible.

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] (blocking-severity) Fix two failing pipeline orchestration tests (mock target mismatch: patch apm_cli.install.phases.resolve.run, not apm_cli.install.phases.resolve). -- outcome=failed on this commit. The resolve-failure->restore path has zero passing test coverage. Tests are new in this PR, not pre-existing failures.
2. [Python Architect] (blocking-severity) Replace suppress(Exception) in restore_update_backups and backup creation with narrow OSError catch and logged warning. -- Three panelists independently confirmed: broad suppression can silently re-create the torn state the PR exists to fix. The performance-expert's EXDEV scenario is a concrete real-world trigger.
3. [DevX UX Expert] Add user-visible feedback on decline/dry-run ('No changes made.') and verbose-mode logging for restore/discard operations. -- Two panelists flagged silent rollback. Safe to defer to a follow-up PR.
4. [Supply Chain Security] Add .apm-update-backup/ to default .gitignore template. -- Crash-orphan risk: SIGKILL between backup creation and finally cleanup leaves directory that git add . could commit.
5. [Doc Writer] Trim CHANGELOG entry to user-observable behavior; remove internal staging-and-rollback mechanism detail. -- Keep a Changelog convention: entries describe what changed for users, not how.

Architecture

classDiagram
    direction LR
    class InstallContext {
        <<Dataclass>>
        +plan_callback Any
        +update_backups dict
        +callback_downloaded dict
        +all_apm_deps list
        +deps_to_install list
        +apm_modules_dir Path
        +apm_dir Path
    }
    class DependencyReference {
        <<ValueObject>>
        +ref_kind str
        +is_local bool
        +get_unique_key() str
        +get_install_path(dir) Path
    }
    class update_backup {
        <<Module>>
        +purge_cached_semver_paths_for_update() dict
        +restore_update_backups(ctx, keep_new) None
        -_sanitize_backup_name(key) str
    }
    class resolve {
        <<Module>>
        +run(ctx) None
        -_resolve_dependencies(ctx) None
    }
    class pipeline {
        <<Module>>
        +run_install_pipeline() InstallResult
    }
    pipeline *-- InstallContext : creates
    pipeline ..> update_backup : try/finally reconcile
    resolve ..> update_backup : purge call
    update_backup ..> InstallContext : reads/writes backups
    update_backup ..> DependencyReference : iterates
    resolve ..> DependencyReference : resolves
    class update_backup:::touched
    class InstallContext:::touched
    class pipeline:::touched
    class resolve:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm update CLI entry"] --> B["pipeline.run_install_pipeline()"]
    B --> C["InstallContext created\nplan_callback stored on ctx"]
    C --> D{"ctx.plan_callback\nis set?"}
    D -->|Yes| E["resolve.py: backup_root =\nctx.apm_dir / .apm-update-backup"]
    D -->|No| F["resolve.py: backup_root = None"]
    E --> G["purge_cached_semver_paths_for_update()\nPath.rename(ip to backup)\nctx.update_backups populated"]
    F --> H["purge_cached_semver_paths_for_update()\nrobust_rmtree(ip) -- delete outright"]
    G --> I["_resolve_dependencies()\nBFS resolver + download_callback"]
    H --> I
    I -->|Exception| J{"ctx.update_backups\nnon-empty?"}
    J -->|Yes| K["restore_update_backups(ctx, keep_new=False)\nrename backups back to install paths"]
    J -->|No| L["re-raise original exception"]
    K --> L
    I -->|Success| M["build_update_plan()"]
    M --> N["committed = False"]
    N --> O["plan_callback(plan)"]
    O -->|"False / SystemExit"| P["finally: restore_update_backups(ctx, keep_new=False)\nrename backups to install paths\nrmtree freshly-added deps"]
    O -->|True| Q["committed = True"]
    Q --> R["finally: restore_update_backups(ctx, keep_new=True)\nrmtree backups (discard)"]
    R --> S["continue to integrate / lockfile write"]

Loading

Recommendation

The core backup/restore design is sound and the PR fixes a real user-facing torn-state bug -- this is exactly the kind of reliability work that builds community trust. However, two issues must be resolved before merge: (1) two new tests confirmed failing on this commit, leaving the most critical new code path (restore on resolve failure) unproven; and (2) suppress(Exception) on the restore path can silently reproduce the exact bug the PR fixes, which three panelists flagged independently. Both fixes are surgical (correct the mock target, narrow the catch to OSError + log). Once those land, this is a clean ship. The remaining findings (silent UX, gitignore, CHANGELOG wording) are safe fast follow-ups.

---

Full per-persona findings

Python Architect

• [recommended] suppress(Exception) in restore_update_backups can silently re-create the torn state this PR fixes at src/apm_cli/install/phases/update_backup.py:157 -- If restore fails (permission error, disk full), user ends up with apm_modules/ missing the dep and no backup -- exactly the torn state the PR exists to prevent. Suggested: Replace with suppress(Exception): in the restore loop with a try/except that logs a warning via ctx.logger.
• [nit] plan_callback on InstallContext is a boolean signal disguised as a callable -- resolve.py only checks for truthiness to decide backup_root. Pragmatic; the docstring explains it.
• [nit] isinstance guards against MagicMock leak (production-test coupling) at src/apm_cli/install/phases/update_backup.py:141 -- Safe; comment is clear.
• [nit] No concurrent-run protection for .apm-update-backup directory -- Consistent with existing no-concurrency-guard behavior; not a regression.

CLI Logging Expert

• [recommended] restore_update_backups is completely silent -- no logging on restore, discard, or fresh-add removal -- In verbose mode, user sees purge but never sees restore. Suggested: Add logger: Any = None parameter; log at verbose_detail level for each restore/discard/removal operation.
• [nit] Purge log says 'cleared' when operation was actually 'moved to backup' -- When backup_root is set, use 'backed up' instead of 'cleared'.

DevX UX Expert

• [recommended] Declined/dry-run update gives zero user feedback about rollback -- User presses 'n', gets bare prompt back with no 'No changes made.' confirmation. Suggested: After restore_update_backups(ctx, keep_new=False), emit a one-liner via existing console helpers.
• [recommended] suppress(Exception) on restore path risks silent corruption -- Failed restore is swallowed; user lands in torn state with no error. Should catch OSError specifically and log a warning.
• [nit] Consider adding .apm-update-backup/ mention to gitignore template documentation -- Ephemeral directory should never be checked in.

Supply Chain Security Expert

• [recommended] Backup directory .apm-update-backup/ is not gitignored -- crash-orphan risk -- SIGKILL between backup creation and finally cleanup leaves directory with old content; git add . could accidentally commit it. Suggested: Add to gitignore auto-update helper, or create .gitignore inside backup root at creation time.
• [nit] suppress(Exception) overly broad for backup and restore operations -- Narrow to suppress(OSError) and log a warning on catch.

OSS Growth Hacker

• [nit] CHANGELOG entry is long -- consider a one-liner lead then keep technical detail as second paragraph.
• [nit] Docs already correctly described the contract this PR fulfills -- worth a release callout to highlight that documented behavior is now actually implemented.

Auth Expert -- inactive

Auth-neutral orchestration change: no changes to credential resolution, token management, AuthResolver invocation, git credential fill, or HTTP/git transport auth paths. Exception handler re-raises after restoring backups, so auth errors propagate unchanged.

Doc Writer

• [recommended] CHANGELOG second sentence describes internal staging-and-rollback mechanism rather than user-observable behavior -- should be cut. Keep a Changelog convention: entries describe what changed for users, not how.
• [nit] 'dependency content untouched' phrase is ambiguous -- use 'manifest, lockfile, and filesystem unchanged' to mirror reference docs vocabulary.
• [nit] update.md reference docs already correctly state the contract -- no update needed; informational.
• [nit] Bug feat(deps): resolve semver ranges on git-source dependencies #1496 reference in docstring without context -- Move to parenthetical or remove; the docstring explains the problem fully without it.

Test Coverage Expert

• [recommended] 2 new pipeline orchestration tests FAIL (confirmed by running pytest) -- patch("apm_cli.install.phases.resolve", mock_resolve) fails with AttributeError; fix: patch("apm_cli.install.phases.resolve.run", side_effect=RuntimeError(...)). NOT a pre-existing failure.
  Evidence (failed, unit): tests/unit/test_install_pipeline_orchestration.py::TestRunInstallPipelineResolveFailureRestoresBackups::test_resolve_exception_triggers_restore_when_backups_staged -- assertion: mock_restore.assert_called_once_with(mock_ctx, keep_new=False)
• [recommended] No integration test exercises the full decline->restore filesystem round-trip through the real pipeline.
  Evidence (missing, integration-with-fixtures): tests/integration/test_update_e2e.py::test_update_dry_run_preserves_apm_modules
• [nit] 16 update_backup unit tests all pass (real filesystem via tmp_path, 0.08s). Strong unit-tier coverage of the new module.

Performance Expert

• [recommended] Cross-filesystem rename silently defeats backup gate at src/apm_cli/install/phases/update_backup.py:110 -- Path.rename() raises EXDEV on different mount points; suppress(Exception) swallows it; dep not added to backups; resolver short-circuits; --update flag silently defeated for that dep. Suggested: Detect EXDEV and fall back to shutil.move(), or log a warning.
• [nit] Happy-path overhead of backup mechanism is negligible vs. network fetch (O(1) rename per dep).
• [nit] robust_rmtree retry overhead acceptable for backup cleanup path.

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

[sergio-sisternes-epam]

The panel found two blocking issues: (1) Two new pipeline orchestration tests in TestRunInstallPipelineResolveFailureRestoresBackups FAIL on this commit -- the mock patches apm_cli.install.phases.resolve instead of apm_cli.install.phases.resolve.run. Fix: change to patch('apm_cli.install.phases.resolve.run', side_effect=RuntimeError(...)). (2) suppress(Exception) in restore_update_backups can silently re-create the torn state this PR exists to fix -- narrow to except OSError and add a logged warning. Both fixes are surgical. The core backup/restore design is sound and this is very close to mergeable.