fix(install): frozen check matches host-qualified private git deps (closes #1996)#2011
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a --frozen validation mismatch in the install pipeline by ensuring manifest dependency keys are derived with the same canonicalization rules as lockfile dependency keys, particularly for non-default git hosts.
Changes:
- Updated manifest-side dependency key derivation in
lockfile_satisfies_manifest/plan logic to usebuild_dependency_unique_key, matching lockfile semantics for host-qualified git deps. - Added regression tests covering host-qualified private SSH git dependencies and default-host (
github.com) SSH git dependencies.
Show a summary per file
| File | Description |
|---|---|
| src/apm_cli/install/plan.py | Switches _dep_ref_key() to use the shared build_dependency_unique_key() helper so manifest and lockfile keys align (including host qualification rules). |
| tests/unit/install/test_plan.py | Adds unit coverage for --frozen satisfaction when lockfile keys include non-default host qualification and when github.com remains implicit. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
Add release-note coverage for the frozen private-host fix, tighten the dependency-key docstring to describe direct delegation, and add a collision guard proving non-default hosts keep distinct lockfile keys. Addresses apm-review-panel follow-ups from doc-writer, oss-growth-hacker, python-architect, auth-expert, and supply-chain-security-expert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
83159a0 to
a8a5a4f
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Single-source key identity via DependencyReference.get_unique_key() is architecturally correct. |
| CLI Logging Expert | 0 | 0 | 0 | No output format regression; the false-positive frozen message is removed. |
| DevX UX Expert | 0 | 0 | 0 | No command surface change; frozen mode now matches user expectations. |
| Supply Chain Security Expert | 0 | 0 | 0 | Host-qualified keys preserve cross-host identity and close the collision risk. |
| OSS Growth Hacker | 0 | 0 | 0 | Changelog captures the enterprise trust fix. |
| Auth Expert | 0 | 0 | 0 | Host identity remains explicit; no token-host regression introduced. |
| Doc Writer | 0 | 0 | 0 | Changelog covered; no docs page drift remains. |
| Test Coverage Expert | 0 | 0 | 0 | Regression traps cover private host, cross-host collision, and github.com default host. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
Ready for maintainer review. All quality gates satisfied, panel follow-ups folded, and no deferred items remain.
Folded in this run
- (panel) Added
CHANGELOG.mdUnreleased Fixed entry for the--frozenprivate Git host fix -- resolved in a8a5a4f. - (panel) Added cross-host collision regression coverage for same
repo_urlon distinct non-default hosts -- resolved in a8a5a4f. - (panel) Replaced the parallel manifest-key mapping with direct
DependencyReference.get_unique_key()delegation -- resolved in a8a5a4f. - (panel) Updated
_dep_ref_keywording from mirror semantics to direct delegation semantics -- resolved in a8a5a4f. - (panel) Reworded the changelog entry around the user-visible spurious rejection symptom -- resolved in a8a5a4f.
Regression-trap evidence (mutation-break gate)
tests/unit/install/test_plan.py::TestLockfileSatisfiesManifest::test_non_default_hosts_keep_distinct_lockfile_keys-- deleted non-default-host qualification inbuild_dependency_unique_key; test FAILED as expected; guard restored.tests/unit/install/test_plan.py::TestLockfileSatisfiesManifest::test_satisfied_when_private_git_dep_lock_key_is_host_qualified-- reverted_dep_ref_keyto the old host-blind body; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/, uv run --extra dev ruff format --check src/ tests/, pylint R0801, and scripts/lint-auth-signals.sh passed after merging origin/main.
CI
gh pr checks 2011 --repo microsoft/apm --watch observed all checks passing on head a8a5a4fe0b7eec956fc3f986880f0e7695424bd2 (after 0 CI fix iteration(s)).
Mergeability status
Captured from gh pr view 2011 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2011 | a8a5a4f |
ship_now | 2 | 5 | 0 | 2 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
2 outer iteration(s); 2 Copilot round(s). Final panel verdict: ship_now.
Ready for maintainer review.
Full per-persona findings
All final in-scope findings were folded in this run. Final panel pass found no remaining recommended follow-ups.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Identity delegation is architecturally sound and keeps dependency identity single-sourced. |
| CLI Logging Expert | 0 | 0 | 0 | No logging regressions; frozen diagnostics now reflect the correct canonical key when drift is real. |
| DevX UX Expert | 0 | 0 | 0 | The lockfile-is-canonical CI promise is restored for private hosts without changing user workflow. |
| Supply Chain Security Expert | 0 | 0 | 0 | Host-qualified identity closes cross-host collision risk with no new dependency-confusion surface. |
| OSS Growth Hacker | 0 | 0 | 0 | The fix strengthens the multi-host enterprise trust story and changelog now names concrete hosts. |
| Auth Expert | 0 | 0 | 0 | No auth regression; production token resolution still flows through AuthResolver. |
| Doc Writer | 0 | 0 | 0 | CHANGELOG and identity docstring are accurate; no README or apm-guide drift remains. |
| Test Coverage Expert | 0 | 0 | 0 | Unit, lightweight integration, and hermetic e2e coverage defend success and wrong-host paths. |
| Performance Expert | 0 | 0 | 0 | No performance risk; the frozen path remains pure in-memory key derivation. |
B = highest-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
direction LR
class DependencyReference {
+repo_url str
+host str or None
+get_unique_key() str
}
class LockedDependency {
+repo_url str
+host str or None
+get_unique_key() str
}
class build_dependency_unique_key {
<<Pure Function - SSOT>>
+__call__(repo_url, host, source, ...) str
}
class _dep_ref_key {
<<Adapter>>
+__call__(dep) str
}
class lockfile_satisfies_manifest {
<<Pure Function>>
+__call__(lockfile, manifest_deps) tuple
}
DependencyReference ..> build_dependency_unique_key : delegates
LockedDependency ..> build_dependency_unique_key : delegates
_dep_ref_key ..> DependencyReference : reads get_unique_key
_dep_ref_key ..> build_dependency_unique_key : fallback for fakes
lockfile_satisfies_manifest ..> _dep_ref_key : computes manifest keys
flowchart TD
A["apm install --frozen"] --> B["lockfile_satisfies_manifest"]
B --> C["_dep_ref_key(dep)"]
C --> D{"get_unique_key available?"}
D -->|yes| E["dep.get_unique_key()"]
D -->|test fake| F["build_dependency_unique_key fallback"]
E --> G{"host is github.com or empty?"}
F --> G
G -->|yes| H["owner/repo"]
G -->|no| I["host/owner/repo"]
H --> J{"key in lockfile?"}
I --> J
J -->|yes| K["frozen install proceeds"]
J -->|no| L["frozen install rejects drift"]
Recommendation
Ship now. The fix is small, the regression trap is proven red/green, the non-network integration suite passes, and CI is green on head 8f74670.
Full per-persona findings
All active panelists reported no remaining findings after the folded follow-ups.
This panel is advisory. Re-apply the panel-review label after addressing feedback to re-run.
# Conflicts: # CHANGELOG.md # tests/integration/test_skill_bundle_live.py
Why/what: apm install --frozen compared manifest git dependencies with a host-blind key while apm.lock.yaml stores non-default-host git dependencies with host-qualified keys, so locked private SSH dependencies were reported missing; this changes the manifest-side frozen key derivation to use the same build_dependency_unique_key canonicalization as lockfile entries and adds regression coverage for non-default-host and github.com SSH dependencies. How to test: uv run --extra dev pytest tests/unit/install/test_plan.py -q; uv run --extra dev pytest tests/unit/install -k 'frozen or lockfile or plan or unique_key or dep_ref' -q; uv run --extra dev pytest tests -k 'frozen or lockfile or plan or unique_key or dep_ref' -q; uv run --extra dev ruff check src/ tests/ --fix; uv run --extra dev ruff format src/ tests/; uv run --extra dev ruff check src/ tests/; uv run --extra dev ruff format --check src/ tests/; uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/; bash scripts/lint-auth-signals.sh. Closes #1996.