Skip to content

fix: honor explicit global install refs (closes #1555)#1559

Merged
danielmeppiel merged 3 commits into
mainfrom
fix/install-g-ref-honored-1555
May 30, 2026
Merged

fix: honor explicit global install refs (closes #1555)#1559
danielmeppiel merged 3 commits into
mainfrom
fix/install-g-ref-honored-1555

Conversation

@danielmeppiel
Copy link
Copy Markdown
Collaborator

Summary

  • update an existing apm.yml dependency when the user installs the same package with an explicit ref
  • add a regression test for unpinned existing entries being pinned by CLI input

Validation

  • uv run --extra dev pytest tests/unit/commands/test_install_resolve_refs.py tests/unit/test_install_command.py tests/unit/commands/test_install_context_and_resolution.py -q
  • uv run --extra dev ruff check src/ tests/
  • uv run --extra dev ruff format --check src/ tests/

Closes #1555

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 30, 2026 15:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes #1555 where apm install -g <pkg>#<ref> was silently ignored if the package was already listed unpinned in the global apm.yml. The fix updates the existing apm.yml entry in-place when the user supplies an explicit ref on the CLI, so the user's intent wins over a stale unpinned entry.

Changes:

  • New helper _manifest_has_different_entry_for_identity to detect a pre-existing manifest entry whose canonical differs from the CLI input.
  • In _resolve_package_references, the "already in deps" branch now also re-merges when an explicit CLI ref differs from the manifest entry, using the CLI-derived structured entry when no other entry was registered.
  • New regression test covering CLI-supplied ref pinning an unpinned manifest entry.
Show a summary per file
File Description
src/apm_cli/commands/install.py Adds helper + branch to update manifest when CLI ref differs from existing identity entry.
tests/unit/commands/test_install_resolve_refs.py Regression test asserting the unpinned dep gets pinned to the CLI ref and dependencies_changed is set.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 0

@danielmeppiel danielmeppiel force-pushed the fix/install-g-ref-honored-1555 branch from 8a9e71e to 36242cf Compare May 30, 2026 16:04
Fold panel follow-ups for the explicit-ref install fix by making validation output distinguish an updated manifest entry from a no-op, using generic manifest persistence wording, and documenting the fix in the changelog.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel danielmeppiel force-pushed the fix/install-g-ref-honored-1555 branch from 36242cf to 849ad2e Compare May 30, 2026 16:07
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_now

PR #1559 now honors explicit install refs for existing global dependencies and keeps the manifest feedback clear when the entry is rewritten.

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

The panel found this fix strategically aligned with the package-manager promise: explicit user intent must win over stale manifest state. The initial follow-ups were all in-scope polish around the same install-ref path, so they were folded into the PR rather than deferred.

Aligned with: pragmatic-as-npm: re-running install with an explicit ref now behaves like a package manager update instead of a silent no-op; portable-by-manifest: the manifest now records the pin the user requested.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 0 Ref-update logic was extracted into apm_cli.install.package_resolution to keep commands/install.py within its architecture budget.
CLI Logging Expert 0 0 0 No raw output paths added; install feedback now distinguishes updated refs from no-op existing deps.
DevX UX Expert 0 0 0 Folded the confusing already in apm.yml and marketplace-specific success wording.
Supply Chain Security Expert 0 0 0 No new trust boundary; explicit refs reduce floating dependency risk.
OSS Growth Hacker 0 0 0 Added a changelog entry for the trust-building silent-footgun fix.
Test Coverage Expert 0 0 0 Regression coverage plus mutation-break checks protect the ref-update feedback and persistence paths.

Folded in this run

  • 849ad2e6 - validation output now says updated ref in apm.yml when an existing manifest entry is rewritten.
  • 849ad2e6 - manifest persistence success wording is generic instead of marketplace-specific.
  • 849ad2e6 - CHANGELOG records the explicit-ref global install fix.
  • 849ad2e6 - ref-update manifest rewrite logic moved into apm_cli.install.package_resolution, restoring the commands/install.py architecture budget under the merge-commit invariant.

Copilot signals reviewed

  • Copilot review 4395565193: no inline findings; classified as no actionable Copilot items.

Validation evidence

  • Local tests: uv run --extra dev pytest tests/unit/test_command_logger.py tests/unit/install/test_package_resolution_persistence.py tests/unit/commands/test_install_resolve_refs.py tests/unit/test_install_command.py tests/unit/commands/test_install_context_and_resolution.py tests/unit/install/test_architecture_invariants.py -q -> 231 passed.
  • Local lint: uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ -> clean.
  • Additional guards: pylint R0801 clean; scripts/lint-auth-signals.sh clean.
  • Mutation-break evidence: removing the updated-ref validation branch made test_validation_pass_existing_updated fail; restoring marketplace-specific persistence wording made test_persist_dependency_list_reports_generic_manifest_update fail.
  • CI: all PR checks green on 849ad2e6cb7f42a060a3d4b0acddfcef34675688.

Mergeability snapshot

#PR head ceo_stance iterations folds deferrals copilot_rounds ci_status mergeable merge_state_status notes
#1559 849ad2e ship_now 1 4 0 2 green MERGEABLE BLOCKED awaiting required review

Recommendation

Ship when the maintainer is ready. There are no deferred items from this shepherd pass.


Full per-persona findings

All actionable panel items were folded in 849ad2e6. No remaining in-scope follow-ups.

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

@danielmeppiel danielmeppiel merged commit bd85d33 into main May 30, 2026
10 checks passed
@danielmeppiel danielmeppiel deleted the fix/install-g-ref-honored-1555 branch May 30, 2026 19:22
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.

apm install -g <pkg>#<ref> silently ignored when global apm.yml has unpinned same package + stale lockfile

2 participants