fix(install): honour spec drift in BFS resolve callback

[sergio-sisternes-epam]

Description

When a package is locked in apm.lock.yaml, changing its #tag or #sha pin in apm.yml (or on the CLI) is silently ignored -- the lockfile's resolved_commit always wins. This makes it impossible to upgrade a package without manually editing the lockfile.

The root cause is the BFS download callback in resolve.py, which unconditionally replaces the user's fragment with the locked commit. The codebase already has a drift.py module with detect_ref_change() and build_download_ref() -- used in 3 of the 4 install code paths (integrate.py, download.py, sources.py) -- but the 4th path (the BFS callback in resolve.py) still used inline logic that ignored spec changes.

Additionally, the --refresh CLI flag was accepted and stored but never read by the install pipeline -- a dead flag.

Approach

• Replace the inline lockfile override in resolve.py with calls to the existing detect_ref_change() and build_download_ref() from drift.py, aligning all four install code paths.
• When drift is detected, mark the dep in ctx.expected_hash_change_deps (in both resolve.py and integrate.py) so the supply-chain content-hash check in sources.py does not treat a legitimate re-resolution as an attack.
• Wire --refresh into update_refs so it triggers re-resolution of all refs.

Non-obvious details

• getattr(ctx, "refresh", False) is used instead of direct attribute access for backward compatibility with any code constructing InstallContext without the refresh field.
• Old lockfiles without resolved_ref are handled conservatively: if the user adds a pin, None != "v1.0.0" triggers a re-download; if neither side has a pin, None == None preserves the lock.

Type of change

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

Testing

• Tested locally
• All existing tests pass (14,994 passed)
• Added tests for new functionality (if applicable)

13 unit tests in tests/unit/install/test_resolve_spec_drift.py covering: tag change, pin added/removed, SHA change, first install, --update regression, expected_hash_change_deps marking, --refresh wiring, and backward compatibility with old lockfiles.

Verified end-to-end against the sergio-sisternes-epam/dde package: changing the spec from #c9e780324... to #v0.4.0 correctly re-resolved from @123a6a9d to @1e4d5b77 and updated the lockfile without triggering a content-hash mismatch error.

[Copilot]

Pull request overview

Fixes the install pipeline behavior where the resolve-phase BFS download callback would always prefer the lockfile resolved_commit, causing manifest #tag / #sha changes to be ignored. The change aligns the resolve phase with the existing drift helpers so spec drift can trigger re-resolution and avoid false-positive supply-chain hash failures.

Changes:

• Update resolve phase BFS download callback to use detect_ref_change() + build_download_ref() and to mark drifted deps in ctx.expected_hash_change_deps.
• Mark ref-drifted deps in integrate phase as expected content-hash changers (to avoid supply-chain false positives).
• Add unit tests covering drift helper behavior used by resolve-phase logic (plus refresh-related expectations).

Reviewed changes

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

File	Description
tests/unit/install/test_resolve_spec_drift.py	Adds unit tests around drift helper behavior and related expectations.
src/apm_cli/install/phases/resolve.py	Replaces inline lockfile override with drift helper calls in the BFS download callback; marks expected hash changes on drift.
src/apm_cli/install/phases/integrate.py	Marks ref-drifted dependencies in expected_hash_change_deps to relax downstream content-hash enforcement when appropriate.

[github-actions]

APM Review Panel: ship_with_followups

fix(install): silent pin-change override is gone -- apm install now honours spec drift in BFS resolve callback, and --refresh is no longer a dead flag (#1473)

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

This PR closes a correctness gap that was a silent trust-killer: a user who changed a pin in apm.yml and ran apm install would see no error, no warning, and the wrong commit installed. The BFS resolve callback was the only install code path that did not consult drift.py, and this fix brings it into alignment. All panelists agree the approach is sound -- the 13 new unit tests validate the drift helpers thoroughly, the supply-chain bypass in expected_hash_change_deps is correctly scoped to user-initiated pin changes and fails closed (false-positive abort, not silent pass-through), and the python-architect confirms the convergence onto drift.py helpers is architecturally correct.

Two convergence signals from cli-logging and devx deserve weight before shipping: spec drift detection is completely silent at runtime, and --refresh activation is equally unconfirmed. Both personas independently flagged this at recommended severity. npm prints "added X packages"; pip prints "Installing ... (upgrade)". APM currently prints nothing, which means the fixed behaviour is indistinguishable from the old broken behaviour from the user's perspective. This is not a blocker -- the correctness is now right -- but shipping without a single diagnostic line risks users filing "did this PR even work?" issues within weeks. The --refresh help text and the apm-guide commands.md skill resource are also out of sync with the flag's newly wired behaviour; these are fast doc fixes.

The test-coverage-expert's two missing-evidence findings are the highest-signal follow-up: the actual fix lives in the BFS download_callback closure, and no non-credential-gated test exercises that wiring. If the closure integration were accidentally reverted, every new unit test would still pass. A synthetic _ResolveCtx fixture test is a one-day write and should land in the same release.

Aligned with: portable-by-manifest: apm.yml is now the authoritative source of truth for ref pins -- a changed pin is honoured on next install rather than silently overridden by the lockfile; secure-by-default: expected_hash_change_deps is correctly scoped, the TOCTOU race fails closed, and --refresh hash-bypass is an explicit user opt-in; oss-community-driven: external contributor diagnosed and fixed a BFS-level correctness bug; pragmatic-as-npm: npm and pip both re-resolve on manifest change -- APM now matches that table-stakes behaviour.

Growth signal. Silent lockfile drift is the kind of bug that makes new users distrust a package manager entirely -- "I changed my pin and nothing happened" is a one-way door to npm. This fix, shipped with a prominent CHANGELOG entry and community attribution (@sergio-sisternes-epam), is a concrete trust signal: APM is legible enough for external contributors to fix BFS-level bugs. Recommended CHANGELOG framing: "Fixed: apm install now re-resolves a dependency when its ref is changed in apm.yml; previously the locked commit always won silently. --refresh is now active (previously a no-op). Contributed by @sergio-sisternes-epam. (#1473)". Amplify in release notes.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	2	Solid convergence of 4 install code paths onto drift.py helpers; lazy import in hot BFS loop and missing context.py field are the two addressable items.
CLI Logging Expert	0	2	1	Spec drift detection is completely silent -- users get no output when their pin change triggers a re-download, and --refresh activation is also unconfirmed.
DevX UX Expert	0	3	2	The core spec-drift fix aligns apm with npm/pip mental models; --refresh is now semantically overloaded without telling users, and silent re-resolution gives no feedback.
Supply Chain Security Expert	0	2	1	The expected_hash_change_deps bypass is correctly scoped to user-initiated pin changes; no blocking issues found, but a lock-free read of the set creates a benign false-positive race and the --refresh bypass semantics deserve a test.
OSS Growth Hacker	0	2	1	Critical silent-failure bug fixed -- strong adoption signal; needs prominent CHANGELOG entry and community amplification to convert trust.
Doc Writer	0	2	1	CHANGELOG entry is missing; one prose claim in update-and-refresh.mdx is now inaccurate after the fix; --refresh flag description is fine.
Test Coverage Expert	0	2	1	13 new unit tests cover the drift helpers thoroughly; the BFS download_callback wire-up in resolve.py and the --refresh ctx-attribute propagation are untested at any non-credential-gated tier.
Auth Expert	--	--	--	No auth surface touched; inactive.

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 a non-credential-gated integration test that exercises the BFS download_callback closure with a synthetic _ResolveCtx and stub downloader, asserting ctx.expected_hash_change_deps contains the drifted dep's key. -- The actual fix lives in the closure wiring; all 13 new unit tests bypass it entirely. A silent revert of the closure integration would pass every existing test. Missing-evidence on a secure-by-default surface -- highest follow-up priority.
2. [Test Coverage Expert] Add a unit test that passes a ctx stub with update_refs=False and refresh=True through the resolve phase expression and asserts update_refs evaluates to True -- proving the getattr wiring, not just build_download_ref directly. -- Current test_refresh_true_acts_like_update_refs bypasses the getattr line it claims to verify. Deleting that line leaves the test green. Missing-evidence on the devx promise that --refresh is honoured.
3. [CLI Logging Expert] Emit a user-visible diagnostic line when spec drift is detected and re-download is triggered (e.g. [!] Spec drift: <dep> pin changed, re-resolving) and confirm --refresh activation with a log line. -- cli-logging and devx converge independently: the fixed behaviour is currently indistinguishable from the old broken behaviour at the terminal. Users cannot confirm the fix took effect without a log.
4. [Doc Writer] Add CHANGELOG entry under [Unreleased] > Fixed, update update-and-refresh.mdx line 67-68 to reflect that entries with a changed ref are re-resolved (not frozen to locked SHA), and update --refresh help text and apm-guide commands.md skill resource. -- oss-growth-hacker, doc-writer, and devx all flag the CHANGELOG gap; doc-writer flags the inaccurate mdx prose. Fast fixes that directly reinforce the trust narrative.
5. [Supply Chain Security Expert] Snapshot expected_hash_change_deps under callback_lock before the sources.py hash-check read, and guard integrate.py .add() with the same lock. Add a test asserting hash-mismatch is still detected when --refresh is set but content is tampered. -- The TOCTOU race fails closed (false-positive abort, not silent pass), but the undocumented --refresh hash-check bypass deserves an explicit test before the next release.

Architecture

classDiagram
    direction LR

    class InstallContext {
        <<ValueObject>>
        +update_refs: bool
        +existing_lockfile: LockFile
        +expected_hash_change_deps: set[str]
        +callback_downloaded: dict
        +refresh: bool
    }
    class InstallContextCmd {
        <<ValueObject>>
        +refresh: bool = False
    }
    note for InstallContext "install/context.py - used by phases (no refresh field; phases use getattr guard)"
    note for InstallContextCmd "commands/install.py - runtime object has refresh: bool = False at line 222"

    class drift {
        <<PureModule>>
        +detect_ref_change(dep_ref, locked_dep, update_refs) bool
        +build_download_ref(dep_ref, lockfile, update_refs, ref_changed) DependencyReference
    }

    class resolve_run {
        <<IOBoundary>>
        +run(ctx: InstallContext) None
        -download_callback(dep_ref, ...) Path
    }
    class resolve_run:::touched

    class integrate_resolve_download_strategy {
        <<Pure>>
        +_resolve_download_strategy(ctx, dep_ref, install_path) tuple
    }
    class integrate_resolve_download_strategy:::touched

    class DependencyReference {
        <<ValueObject>>
        +reference: str
        +get_unique_key() str
    }

    class LockedDependency {
        <<ValueObject>>
        +resolved_ref: str
        +resolved_commit: str
    }

    class LockFile {
        +get_dependency(key) LockedDependency
    }

    class sources_FreshDependencySource {
        <<ConcreteStrategy>>
        +acquire(dep_ref, ctx) PackageInfo
    }
    note for sources_FreshDependencySource "Collect-then-render: reads ctx.expected_hash_change_deps to gate supply-chain hash check"

    resolve_run ..> drift : calls detect_ref_change + build_download_ref
    integrate_resolve_download_strategy ..> drift : calls detect_ref_change
    resolve_run ..> InstallContext : reads + mutates expected_hash_change_deps
    integrate_resolve_download_strategy ..> InstallContext : reads + mutates expected_hash_change_deps
    sources_FreshDependencySource ..> InstallContext : reads expected_hash_change_deps
    drift ..> DependencyReference : reads reference
    drift ..> LockedDependency : reads resolved_ref / resolved_commit
    drift ..> LockFile : calls get_dependency
    InstallContext *-- LockFile : existing_lockfile
    InstallContextCmd --|> InstallContext : runtime subtype (duck-typed)

    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["apm install CLI"]) --> B["resolve.run(ctx)"]
    B --> C["Load apm.lock.yaml"]
    B --> D["update_refs = ctx.update_refs OR getattr(ctx,'refresh',False)"]
    D --> E["BFS walk of dependency graph"]
    E -->|"per dep"| F["download_callback(dep_ref)"]
    F --> G{"dep_ref.is_local?"}
    G -->|yes| H["_copy_local_package()"]
    G -->|no| I["drift.detect_ref_change(dep_ref, locked_dep, update_refs)"]
    I -->|"ref changed"| J["[lock] ctx.expected_hash_change_deps.add(key)"]
    I --> K["drift.build_download_ref(dep_ref, lockfile, update_refs, ref_changed)"]
    K -->|"locked and stable"| L["download_dep = locked_commit"]
    K -->|"drift or update"| M["download_dep = manifest ref"]
    L & M --> N["downloader.download_package(download_dep)"]
    N --> O["integrate._resolve_download_strategy(ctx, dep_ref)"]
    O --> P["drift.detect_ref_change -- idempotent add to expected_hash_change_deps"]
    P --> Q["sources_FreshDependencySource.acquire()"]
    Q --> R{"key in expected_hash_change_deps?"}
    R -->|yes| S["Skip hash block (legitimate re-resolution)"]
    R -->|no| T["Supply-chain content-hash check"]

Loading

sequenceDiagram
    actor User
    participant CLI as commands/install.py
    participant Resolve as phases/resolve.py
    participant Drift as drift.py
    participant Downloader as github_downloader.py
    participant Sources as install/sources.py

    User->>CLI: apm install (ref pin changed in apm.yml)
    CLI->>Resolve: run(ctx) with refresh/update_refs
    Resolve->>Resolve: load apm.lock.yaml
    loop BFS per dep
        Resolve->>Drift: detect_ref_change(dep_ref, locked_dep)
        Drift-->>Resolve: ref_changed=True
        Resolve->>Resolve: expected_hash_change_deps.add(key)
        Resolve->>Drift: build_download_ref(..., ref_changed=True)
        Drift-->>Resolve: dep_ref (manifest ref, not locked commit)
        Resolve->>Downloader: download_package(dep_ref)
        Downloader-->>Resolve: install_path + resolved_sha
    end
    Resolve->>Sources: acquire(dep_ref, ctx)
    Sources->>Sources: check expected_hash_change_deps -- skip hash block
    Sources-->>CLI: PackageInfo with new content_hash
    CLI-->>User: [+] upgraded to new ref

Loading

Recommendation

Ship PR #1473 -- the correctness fix is sound, the approach is architecturally aligned, and the supply-chain bypass is correctly scoped and fails closed. Gate the release on three fast follow-ups that should land before or with the next minor: (1) the CHANGELOG entry and doc fixes (doc-writer, devx), which are a PR-day write; (2) the BFS closure integration test (test-coverage-expert), which prevents silent regression of the actual fix; and (3) a diagnostic log line when drift is detected (cli-logging/devx), so users can confirm the fix is active. The remaining items -- lock-protection cleanup, context.py refresh field, and the --refresh hash-bypass test -- are recommended but can trail into the next patch. Attribution to @sergio-sisternes-epam should be explicit in the CHANGELOG and release notes.

---

Full per-persona findings

Python Architect

• [recommended] Lazy import inside the BFS download_callback hot loop should be hoisted at src/apm_cli/install/phases/resolve.py:275
  The line from apm_cli.drift import build_download_ref, detect_ref_change appears inside download_callback(), which is invoked once per transitive dependency. Python caches modules after first load so correctness is unaffected, but each invocation performs a sys.modules dict lookup and inhibits static analysis.
  Suggested: Move import to the function-scope lazy-import block at the top of run().

• [recommended] refresh belongs in install/context.py InstallContext, not accessed via getattr at src/apm_cli/install/context.py:58
  The phases' declared type InstallContext (from context.py) lacks refresh. Any future phase author reading context.py has no signal that refresh exists. Adding refresh: bool = False to install/context.py makes the contract explicit and lets mypy/pyright catch absent setters.
  Suggested: Add refresh: bool = False to InstallContext, then replace getattr(ctx, "refresh", False) with ctx.refresh.

• [nit] Double-add to expected_hash_change_deps (resolve.py and integrate.py) is benign but worth a comment at src/apm_cli/install/phases/integrate.py:94
  Set semantics make double-add safe, but a short comment noting the intentional redundancy saves future maintainers.
  Suggested: Add: # resolve.py's BFS callback may have already added this; set semantics make double-add safe.

• [nit] Design pattern: PR extends the Collect-then-render pattern correctly -- expected_hash_change_deps acts as a drift signal set accumulated during resolve and consumed by sources.py.

CLI Logging Expert

• [recommended] No user-visible output when spec drift is detected and re-download is triggered at src/apm_cli/install/phases/resolve.py:287
  When _ref_changed is True, the code silently queues a re-download. The user changed a pin expecting an upgrade but receives no feedback that drift was noticed or what new ref will be fetched.
  Suggested: After the if _ref_changed: block, emit: logger.verbose_detail(f" [!] Spec drift: {dep_ref.get_unique_key()} pin changed, re-resolving")

• [recommended] --refresh flag activation is silent; user has no confirmation it took effect at src/apm_cli/install/phases/resolve.py:190
  resolve.py:190 folds ctx.refresh into update_refs without any log. A user who passes --refresh and sees the same output as a plain install has no confirmation the flag was honoured.
  Suggested: After line 190: if getattr(ctx, 'refresh', False) and logger: logger.verbose_detail('[*] --refresh: re-resolving all refs')

• [nit] integrate.py:93 expected_hash_change_deps add has no accompanying log -- if resolve.py gets a drift log, integrate.py should mirror it.

DevX UX Expert

• [recommended] No user-visible feedback when spec drift is detected and re-download triggered at src/apm_cli/install/phases/resolve.py:287
  Without feedback, users cannot distinguish new correct behaviour from the old silent-ignore bug -- especially concerning since for months they may have assumed the old behaviour was correct.
  Suggested: Emit: [~] owner/pkg v1.0.0 -> v2.0.0 (ref drift, re-resolving) in default (non-verbose) output.

• [recommended] --refresh help text does not mention ref re-resolution; stated scope is now a subset of actual behaviour at src/apm_cli/commands/install.py:1072
  After this PR, --refresh also sets update_refs=True, triggering re-resolution of all pinned refs. The help text still says only "Bypass the persistent cache." The discoverability contract is broken.
  Suggested: Update help to: "Re-fetch all dependencies from upstream and re-resolve all ref pins. Use --update for interactive upgrade planning."

• [recommended] --refresh is absent from commands.md skill resource (apm-guide package), now out of sync at packages/apm-guide/.apm/skills/apm-usage/commands.md:13
  Now that --refresh is wired, any agent or user consulting that skill will be unaware of the flag and its effect.
  Suggested: Add --refresh to the flag list: "--refresh re-fetch all deps from upstream and re-resolve all ref pins."

• [nit] getattr(ctx, 'refresh', False) is fragile -- if the field is ever renamed, the flag silently has no effect again (same class of bug this PR is fixing).

• [nit] The --update vs --refresh distinction is unexplained in any user-facing text; a one-sentence note to --update help would help.

Supply Chain Security Expert

• [recommended] expected_hash_change_deps read in sources.py is not lock-protected, creating a TOCTOU race with the lock-protected write in download_callback at src/apm_cli/install/phases/resolve.py:289
  resolve.py adds to the set under callback_lock, but sources.py reads it without holding any lock. Failure mode is false-positive abort ("Content hash mismatch") -- fails closed, not silently. integrate.py writes without any lock at all.
  Suggested: Hold callback_lock (or a snapshot) for reads in sources.py hash-check block. Guard integrate.py .add() with the same lock.

• [recommended] --refresh + update_refs=True suppresses expected_hash_change_deps population AND disables hash check -- implicit undocumented bypass of supply-chain content-hash check at src/apm_cli/install/phases/resolve.py:184
  When --refresh is active, detect_ref_change() always returns False (update_refs=True short-circuits), so no dep is added to expected_hash_change_deps. Sources.py also skips hash check entirely when ctx.update_refs is True. No test asserts a tampered content hash is still detected when --refresh is set.
  Suggested: Emit a warning when --refresh is used. Add a test asserting hash-mismatch aborts install even after a ref change with --refresh.

• [nit] detect_ref_change() does not detect host changes -- documented as known non-goal but worth a TODO linking to a future enhancement issue.

OSS Growth Hacker

• [recommended] This fix must appear prominently in CHANGELOG -- "silent lockfile drift" is a trust-killer for new users comparing apm to npm/pip.
  Suggested: Open with the user-facing pain before the technical fix. Include a before/after example snippet.

• [recommended] --refresh flag was silently accepted but did nothing -- deserves its own separate CHANGELOG bullet.
  Suggested: "--refresh flag now actually refreshes resolution (previously accepted but had no effect)."

• [nit] Community PR from @sergio-sisternes-epam fixing a critical silent bug is a strong OSS signal -- add contributor attribution in CHANGELOG and consider a release note mention.

Auth Expert -- inactive

No auth-surface files touched; changes are confined to BFS lockfile drift detection and ref resolution in install/resolve.py and install/integrate.py.

Doc Writer

• [recommended] No CHANGELOG entry for PR fix(install): honour spec drift in BFS resolve callback #1473 at CHANGELOG.md
  Silent pin-change override is a user-visible correctness bug; it warrants a Fixed entry.
  Suggested: Add under [Unreleased] > Fixed: "apm install now re-resolves a dependency when its ref is changed in apm.yml; previously the locked commit always won, silently ignoring the edit. --refresh is now active (previously accepted but had no effect). (fix(install): honour spec drift in BFS resolve callback #1473)"

• [recommended] update-and-refresh.mdx claims "existing entries stay on their locked SHAs" -- no longer true when user edits the pin at docs/src/content/docs/consumer/update-and-refresh.mdx
  Line 67-68 will mislead users reading the page after this fix ships.
  Suggested: "New entries and entries whose ref was changed in apm.yml are resolved and locked; entries whose spec is unchanged stay on their locked SHAs."

• [nit] --refresh flag description in install.md is already accurate post-fix; no action needed.

Test Coverage Expert

• [recommended] BFS download_callback in resolve.py never exercised by a non-credential-gated test at src/apm_cli/install/phases/resolve.py
  The 13 new unit tests call detect_ref_change/build_download_ref from drift.py directly; they never instantiate a resolve phase or invoke the closure. The only e2e test covering the full chain is gated on requires_github_token. If the wiring inside download_callback were reverted, every new unit test would still pass.
  Evidence (missing, integration-with-fixtures): tests/unit/install/phases/test_resolve_phase_spec_drift.py::test_download_callback_populates_expected_hash_change_deps_on_drift -- proves: When apm install detects spec drift, the install pipeline re-resolves the dep and marks it so sources phase does not raise a false supply-chain alarm.
  Suggested: Add a test with a synthetic _ResolveCtx and stub downloader that calls run_phase(ctx) and asserts ctx.expected_hash_change_deps contains the drifted dep's key.

• [recommended] --refresh ctx-attribute propagation is tested by bypassing the wiring it claims to verify at tests/unit/install/test_resolve_spec_drift.py
test_refresh_true_acts_like_update_refs manually sets update_refs=True and calls build_download_ref directly -- never touches the getattr(ctx, 'refresh', False) line in resolve.py. Deleting that line leaves the test green.
  Evidence (missing, unit): tests/unit/install/phases/test_resolve_phase_spec_drift.py::test_refresh_flag_on_ctx_propagates_to_update_refs -- proves: apm install --refresh re-resolves all refs even when --update-refs was not passed.

• [nit] No regression-trap test proves the pre-fix silent-override behaviour no longer occurs -- nothing proves resolve.py actually passes ref_changed=True when drift is present. A regression to the pre-fix code path would be invisible to the new tests.
  Evidence (missing, integration-with-fixtures): proves: A locked install with a changed manifest ref re-downloads at the manifest ref, not silently at the old locked commit.

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

Generated by PR Review Panel for issue #1473 · ● 3.4M · ◷