feat(deps): resolve semver ranges on git-source dependencies#1496
Conversation
…inst repo tags Closes #1488. Adds a GitSemverResolver that, when a git-source dependency carries a semver range (e.g. `^1.2.0`) as its `ref:`, resolves the range against the remote's tags at install time and records the concrete tag in the lockfile. Composition over the existing RefResolver + marketplace.semver + marketplace.tag_pattern primitives. Pattern conventions: - Primary patterns: `v{version}`, `{name}--v{version}` (matches the Claude Code / PR #1422 convention) - Bare `{version}` is a third-pass fallback only. Lockfile additions (v2, additive — old readers preserve unknown keys via a forward-compat allowlist in from_dict / to_dict): - `constraint`: original semver range from apm.yml - `resolved_tag`: concrete tag selected - `resolved_at`: RFC 3339 resolution timestamp Lockfile replay: on subsequent installs, if the manifest constraint still matches the locked constraint, the resolved tag is replayed without a network call. `apm install --update` or a manifest change triggers re-resolution. Drift detection: `detect_ref_change` now treats a semver-range manifest ref vs. a literal locked tag as 'no drift' when the locked constraint equals the manifest range, mirroring the existing `_registry_range_covers_locked_version` path. Tests: 40 new tests across 4 files - tests/unit/deps/test_git_semver_resolver.py (12 tests) - tests/unit/deps/test_lockfile_git_semver.py (7 tests) - tests/unit/models/test_dependency_reference_semver_routing.py (10) - tests/unit/install/test_git_semver_wiring.py (11 tests) Full unit suite: 15572 passed. Docs updated: manifest-schema (semver ref:), lockfile-spec (new fields table rows), consumer/manage-dependencies (new section). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds git-source semver range support so ref: in apm.yml can be a semver constraint (e.g. acme/widget#^1.2.0) that is resolved against remote git tags and then pinned deterministically in the lockfile.
Changes:
- Introduces
GitSemverResolverto resolve semver constraints against remote tags (with default tag patterns + bare-version fallback). - Adds semver routing via
DependencyReference.ref_kind, wires resolution into the install resolve phase, and updates drift detection semantics. - Extends lockfile entries (v2, additive) to record
constraint,resolved_tag, andresolved_at, and updates docs + unit tests for the new behavior.
Show a summary per file
| File | Description |
|---|---|
| src/apm_cli/deps/git_semver_resolver.py | New resolver that maps semver constraints to concrete git tags + SHAs. |
| src/apm_cli/models/dependency/reference.py | Adds ref_kind to route semver refs vs literal refs. |
| src/apm_cli/install/phases/resolve.py | Wires git-semver resolution into BFS callback before git operations. |
| src/apm_cli/install/sources.py | Plumbs git-semver resolution into InstalledPackage so lockfile re-emits the fields. |
| src/apm_cli/install/context.py | Adds git_semver_resolutions cache on install context. |
| src/apm_cli/drift.py | Treats semver-range manifest refs vs locked literal tags as non-drift when constraints match. |
| src/apm_cli/deps/lockfile.py | Adds semver fields to LockedDependency, preserves unknown keys, bumps lockfile to v2 when needed. |
| src/apm_cli/deps/installed_package.py | Adds git_semver_resolution plumbing to installed package model. |
| tests/unit/deps/test_git_semver_resolver.py | Unit tests for resolver behavior (patterns, prereleases, errors, fallback). |
| tests/unit/models/test_dependency_reference_semver_routing.py | Unit tests for ref_kind routing and regression traps. |
| tests/unit/install/test_git_semver_wiring.py | Unit tests for install wiring + drift semantics for semver git refs. |
| tests/unit/deps/test_lockfile_git_semver.py | Unit tests for lockfile serialization/versioning/forward-compat behavior. |
| docs/src/content/docs/reference/manifest-schema.md | Documents semver ranges for git ref: and lockfile replay behavior. |
| docs/src/content/docs/reference/lockfile-spec.md | Documents new lockfile fields for git-source semver resolution. |
| docs/src/content/docs/consumer/manage-dependencies.md | Adds user-facing guidance/examples for semver ranges on git deps. |
Copilot's findings
- Files reviewed: 15/16 changed files
- Comments generated: 5
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 2 | Well-composed module with correct composition-over-inheritance; one finding on BFS-callback mutation ordering. |
| CLI Logging Expert | 0 | 2 | 2 | NoMatchingTagError is well-crafted; missing verbose-mode breadcrumb so agents cannot trace why a ref was rewritten. |
| DevX UX Expert | 0 | 2 | 2 | Solid npm-style lockfile-replay semantics; cli-commands.md and skill-resource sync make the change incomplete by the project's own standard. |
| Supply Chain Security | 0 | 1 | 1 | Commit-SHA pinning on replay is correct; recommend defense-in-depth SHA format validation on lockfile replay. |
| OSS Growth Hacker | 0 | 1 | 2 | Strong beyond-parity feature with good docs hero example; recommend CHANGELOG entry and release-notes story angle. |
| Auth Expert | 1 | 1 | 1 | git ls-remote for semver resolution bypasses AuthResolver; private repos fail in CI without a system credential helper. |
| Doc Writer | 0 | 4 | 2 | Doc changes are accurate; apm-guide skill, outdated.md, and cross-links don't yet acknowledge the new range syntax. |
| Test Coverage Expert | 0 | 1 | 0 | 40 unit tests pass; no integration-with-fixtures test exercises full install+lockfile round-trip (tier-floor gap). |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Auth Expert] (blocking-severity) Thread AuthResolver token into RefResolver construction (resolve.py:129) so ls-remote on private repos uses GITHUB_APM_PAT/ADO_APM_PAT instead of relying on system credential helpers. -- Correctness regression: private-repo semver resolution fails in CI/containers. User promise that ls-remote behaves like clone is violated without this fix.
- [Test Coverage Expert] Add integration-with-fixtures test: local bare repo with tagged commits, full install pipeline, assert lockfile replay determinism + zero ls-remote on second run. -- Tier-floor matrix requires integration coverage for install-pipeline + lockfile-determinism. All 40 unit tests mock at the boundary they defend. Evidence outcome=missing on a secure-by-default surface elevates this above opinion findings.
- [DevX UX Expert + Doc Writer] Sync apm-guide/.apm/skills/apm-usage/commands.md and dependencies.md with new semver-range syntax so the in-product agent teaches valid syntax. -- Two independent panelists converge: project rule states CLI changes not in cli-commands.md are incomplete; in-product agent will tell users the new syntax is invalid until these files are updated.
- [OSS Growth Hacker] Add CHANGELOG entry for semver-range git resolution -- headline beyond-parity feature needs raw material for release notes and comparison table. -- No CHANGELOG entry means no release narrative anchor. The moat story ('first package manager to resolve semver on git deps') cannot be told without it.
- [CLI Logging Expert] Fix NoMatchingTagError surfacing as 'Failed to download' -- verb mismatch confuses resolution vs download failure semantics. -- Users debugging constraint mismatches will see 'download failed' when no download was attempted. Erodes trust in error messages at a key troubleshooting moment.
Architecture
classDiagram
direction LR
class GitSemverResolver {
<<Composition>>
+resolve(owner_repo, package_name, constraint) GitSemverResolution
-_pick_best(refs, package_name, patterns, constraint)
-_render_version(version) str
}
class RefResolver {
<<Transport+Cache>>
+list_remote_refs(owner_repo) list~RemoteRef~
}
class GitSemverResolution {
<<ValueObject / frozen dataclass>>
+constraint str
+resolved_version str
+resolved_tag str
+resolved_sha str
+matched_pattern str
+resolved_at str
}
class DependencyReference {
+reference str
+ref_kind str|None
+repo_url str
+host str
}
class LockedDependency {
+constraint str|None
+resolved_tag str|None
+resolved_at str|None
+_unknown_fields dict
+to_dict() dict
+from_dict(data) LockedDependency
}
class InstalledPackage {
+git_semver_resolution GitSemverResolution|None
}
class InstallContext {
+git_semver_resolutions dict
}
class NoMatchingTagError {
<<Exception>>
+summary str
+hint str
}
class SemVer {
<<ValueObject>>
+major int
+minor int
+patch int
}
GitSemverResolver *-- RefResolver : delegates transport
GitSemverResolver ..> SemVer : compares
GitSemverResolver ..> GitSemverResolution : returns
GitSemverResolver ..> NoMatchingTagError : raises
DependencyReference ..> GitSemverResolver : routed when ref_kind=semver
InstalledPackage o-- GitSemverResolution : carries
LockedDependency ..> GitSemverResolution : serializes from
InstallContext o-- GitSemverResolution : stashes per dep_key
class GitSemverResolver:::touched
class GitSemverResolution:::touched
class DependencyReference:::touched
class LockedDependency:::touched
class InstalledPackage:::touched
class InstallContext:::touched
class NoMatchingTagError:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm install / BFS download_callback"] --> B{"dep_ref.ref_kind == semver?"}
B -- No --> G["Existing literal-ref path"]
B -- Yes --> C{"Lockfile has matching constraint?"}
C -- Yes & !update_refs --> D["[Replay] Rebuild GitSemverResolution from lockfile, no network call"]
C -- No / --update --> E["[NET] RefResolver.list_remote_refs / git ls-remote"]
E --> F["GitSemverResolver._pick_best (iter_semver_tags + satisfies_range)"]
F --> H{"Winner found?"}
H -- No primary --> I["Retry with FALLBACK_BARE_PATTERN"]
I --> H2{"Winner found?"}
H2 -- No --> J["raise NoMatchingTagError (summary + hint)"]
H2 -- Yes --> K["Build GitSemverResolution"]
H -- Yes --> K
D --> L["[LOCK] ctx.git_semver_resolutions[dep_key] = resolution"]
K --> L
L --> M["dep_ref.reference = resolved_tag (in-place rewrite)"]
M --> N["detect_ref_change (sees literal tag now)"]
N --> O["Proceed to clone / checkout"]
O --> P["install/sources.py acquire()"]
P --> Q["InstalledPackage(git_semver_resolution=...)"]
Q --> R["[FS] LockFile.from_installed_packages emits constraint + resolved_tag + resolved_at"]
sequenceDiagram
participant User
participant CLI as apm install
participant BFS as download_callback
participant GSR as GitSemverResolver
participant RR as RefResolver
participant Remote as git remote
participant LF as LockFile
User->>CLI: apm install (first time)
CLI->>BFS: dep_ref with ref=^1.2.0
BFS->>BFS: ref_kind == semver, no lockfile match
BFS->>GSR: resolve(acme/widget, ^1.2.0)
GSR->>RR: list_remote_refs(acme/widget)
RR->>Remote: git ls-remote
Remote-->>RR: refs/tags/v1.5.3, v1.2.0, v2.0.0...
RR-->>GSR: [RemoteRef...]
GSR->>GSR: iter_semver_tags + satisfies_range
GSR-->>BFS: GitSemverResolution(tag=v1.5.3)
BFS->>BFS: dep_ref.reference = v1.5.3
BFS->>CLI: clone v1.5.3
CLI->>LF: emit constraint=^1.2.0, resolved_tag=v1.5.3
Note over User,LF: Subsequent install (lockfile replay)
User->>CLI: apm install (fresh clone)
CLI->>BFS: dep_ref with ref=^1.2.0
BFS->>BFS: ref_kind == semver
BFS->>LF: get_dependency() -> constraint=^1.2.0 matches
BFS->>BFS: Rebuild resolution from lockfile (no network)
BFS->>BFS: dep_ref.reference = v1.5.3
BFS->>CLI: clone v1.5.3 (deterministic)
Recommendation
Ship once the auth-expert blocking fix is folded into this PR (thread AuthResolver token into RefResolver at resolve.py:129). The shepherd convention is bias-toward-folding for blocking findings, and this fix is scoped to a single call-site with no design ambiguity. Remaining follow-ups (integration test, skill-resource sync, CHANGELOG, error-verb fix) are high-signal but can land same-wave or immediately post-merge without regressing users. The strategic foundation is correct -- merge fast, amplify the moat story.
Full per-persona findings
Python Architect
- [recommended] dep_ref.reference mutation outside callback_lock creates a subtle ordering assumption at
src/apm_cli/install/phases/resolve.py:488
In resolve.py:487-488,dep_ref.reference = _semver_resolution.resolved_tagis written OUTSIDE thecallback_lockscope. Whiledep_refis local to each BFS invocation and not shared across threads (so no race), placing the dict write inside the lock and the dep_ref mutation outside creates a non-obvious contract.
Suggested: Movedep_ref.reference = _semver_resolution.resolved_taginside thewith callback_lock:block, or add an explicit comment. - [nit] _maybe_resolve_git_semver instantiates a new RefResolver on every fresh-resolution call at
src/apm_cli/install/phases/resolve.py:136
If multiple deps share the same host and the RefResolver's internal cache is instance-scoped, each dep pays a separate ls-remote call even when the remote is identical. Performance nit for monorepos. - [nit] package_name parameter in iter_semver_tags is unused and silenced with _ = package_name at
src/apm_cli/deps/git_semver_resolver.py:197
The docstring says 'reserved for future {name} expansion' but the placeholder pattern is semantically confusing.
CLI Logging Expert
- [recommended] No verbose-mode output when git-semver resolution succeeds at
src/apm_cli/install/phases/resolve.py:480
The resolve phase emits verbose_detail() for other resolution steps, but _maybe_resolve_git_semver silently rewrites dep_ref.reference without any verbose trace. Agent or human with --verbose has no breadcrumb explaining the constraint->tag mapping.
Suggested: After successful resolution, emit a verbose_detail with constraint -> resolved_tag (matched pattern). - [recommended] NoMatchingTagError is caught by generic 'Failed to download' handler -- misleading verb at
src/apm_cli/install/phases/resolve.py:536
Users see 'Failed to download dependency acme/widget: No tags on acme/widget satisfy ^9.0.0...' which conflates resolution and download phases.
Suggested: Catch NoMatchingTagError before the generic handler with a 'No matching tag for ...' message. - [nit] Error message says 'Tags considered: ...' -- could say 'Tags inspected:' for consistency with docs at
src/apm_cli/deps/git_semver_resolver.py:300 - [nit] Hint message ASCII safety check (defensive) at
src/apm_cli/deps/git_semver_resolver.py:309
DevX UX Expert
- [recommended] cli-commands.md not updated -- the canonical CLI reference does not mention semver-range support on ref: at
docs/src/content/docs/reference/cli-commands.md
Project rule: 'If a CLI change is not reflected in cli-commands.md in the same PR, that change is incomplete by definition.' - [recommended] Skill resources (packages/apm-guide/.apm/skills/apm-usage/commands.md) not synced with new semver-range capability at
packages/apm-guide/.apm/skills/apm-usage/commands.md
Rule 4 in the DevX UX persona requires shipped skill resources to stay in sync with the docs. - [nit] NoMatchingTagError hint does not mention the tag patterns that were tried at
src/apm_cli/deps/git_semver_resolver.py:404 - [nit] Bare '1.2.3' as ref silently routes to semver resolver -- may surprise users with a literal '1.2.3' tag at
docs/src/content/docs/consumer/manage-dependencies.md
Supply Chain Security Expert
- [recommended] Lockfile replay trusts resolved_commit without validating it is a 40-char hex SHA at
src/apm_cli/install/phases/resolve.py
Not exploitable for code execution (git would reject), but defense-in-depth at the trust boundary produces a clearer error message than a downstream git failure. - [nit] Document tag-mutation threat model in the lockfile-spec or security.md at
docs/src/content/docs/enterprise/security.md
Implementation correctly defends (replay pins SHA not tag), but the contract is undocumented.
OSS Growth Hacker
- [recommended] Add a CHANGELOG entry mining the story angle for release notes
Headline feature ('npm-style semver ranges for git deps -- no more manual re-pinning') has no raw material for release narrative without a CHANGELOG entry. - [nit] Consider adding a semver-range example to the README hero YAML in a follow-up at
README.md - [nit] The manage-dependencies docs section title 'Pin a semver range' undersells the feature at
docs/src/content/docs/consumer/manage-dependencies.md
Auth Expert
- [blocking] RefResolver constructed without token -- git-semver ls-remote bypasses AuthResolver entirely at
src/apm_cli/install/phases/resolve.py:129
APM's auth contract (every remote operation must go through AuthResolver) is violated. RefResolver(host=dep_ref.host) is constructed without token=. The RefResolver then calls git ls-remote on a bare HTTPS URL. For private repos relying on GITHUB_APM_PAT / GITHUB_TOKEN / ADO_APM_PAT (standard CI path), no token reaches the git subprocess. System credential helpers may paper over this on developer machines, but CI environments will get a 401/403 on any private repo using semver ranges.
Suggested: Resolve the token via ctx.auth_resolver before constructing RefResolver:auth_ctx = ctx.auth_resolver.resolve_for_dep(dep_ref); token = auth_ctx.token if auth_ctx else None; ref_resolver = RefResolver(host=dep_ref.host, token=token). - [recommended] No fallback-to-credential-fill semantics on ls-remote failure for semver resolution at
src/apm_cli/marketplace/ref_resolver.py:210 - [nit] GHE (*.ghe.com) and ADO hosts require auth unconditionally -- no unauthenticated fallback should be attempted at
src/apm_cli/marketplace/ref_resolver.py:153
Doc Writer
- [recommended] apm-guide skill
dependencies.mdnot updated for semver-range git refs atpackages/apm-guide/.apm/skills/apm-usage/dependencies.md
The skill file enumerates ref shapes (pinned tag, branch, SHA, object form) and is the source the in-product agent reasons from. Per docs-update instructions this is the named twin of the manage-dependencies page. - [recommended]
apm outdatedreference does not classify semver-range git-source deps atdocs/src/content/docs/reference/cli/outdated.md
Range-pinned git deps (^1.2.0) have a materially different comparison rule (compare lockedresolved_tagagainst highest remote tag matching range). Consumer with^1.2.0finds no row that fits. - [recommended] Missing cross-links between the three changed pages at
docs/src/content/docs/consumer/manage-dependencies.md
Reader who wants to interpretconstraint/resolved_tag/resolved_atin a real lockfile has to grep. - [recommended] Consumer page doesn't mention that
resolved_refis rewritten to the concrete tag atdocs/src/content/docs/consumer/manage-dependencies.md
Careful reader will misread current wording as 'soresolved_refstill says ^1.2.0' and then be surprised when their lockfile diff showsresolved_ref: v1.5.3. - [nit] Manifest-schema entry is one dense 6-line paragraph at
docs/src/content/docs/reference/manifest-schema.md - [nit] Lockfile-spec
constraintrow mixes definition with design rationale atdocs/src/content/docs/reference/lockfile-spec.md
Test Coverage Expert
- [recommended] No integration-with-fixtures test for git-semver install pipeline + lockfile replay determinism at
tests/integration/test_install_git_semver_e2e.py
The PR adds four unit test files (40 tests total, all passing) that thoroughly cover resolver logic, lockfile serialization, install wiring routing, and drift detection. However, every test mocks RefResolver at the boundary. Tier-floor matrix requires integration-with-fixtures for both the install-pipeline and lockfile-determinism surfaces.
Suggested: Add one integration test that creates a fixture manifest with a semver-range git dep pointing at a local bare repo with tagged commits, runs the install pipeline with real RefResolver, asserts lockfile contains constraint + resolved_tag + resolved_commit, then runs install again and asserts no ls-remote call (replay path).
Proof (missing at):tests/integration/test_install_git_semver_e2e.py::test_git_semver_install_roundtrip_replays_from_lockfile-- proves: apm install with a git-semver dep writes lockfile with constraint+resolved_tag, and second install replays from lockfile without network [devx,portability-by-manifest]
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
… panel follow-ups Folds the blocking finding and three high-signal follow-ups from the apm-review-panel advisory on PR #1496: 1. **[auth-expert, blocking]** Thread AuthResolver token through _maybe_resolve_git_semver so the ls-remote call for semver-tag enumeration goes through the same auth path as the downstream clone. Without this, private-repo semver-range deps fell back to the system git credential helper, which is absent in CI (GitHub Actions, ADO pipelines, containers) where GITHUB_APM_PAT / ADO_APM_PAT are the only credential source. Adds TestMaybeResolveGitSemverAuthThreading (4 regression-trap tests) covering: token threading on the fresh-resolution path, None-token fallback for callers without an auth_resolver, exception-safe fallback when resolve_for_dep raises, and the lockfile-replay path correctly skipping auth resolution. Mutation-break gate verified: removing 'token=token' from the RefResolver construction makes test_token_threaded_into_ref_resolver_when_auth_resolver_supplied fail with 'assert None == ghp_testtoken_abc123'. 2. **[cli-logging, recommended]** NoMatchingTagError is now surfaced with a 'No matching tag for ...' message instead of being caught by the generic 'Failed to download dependency ...' handler. The dep_ref is rewritten to a concrete tag BEFORE clone, so a no-match means the download never started -- the old wording misled users debugging an unsatisfied constraint. 3. **[devx-ux + doc-writer, recommended converging signal]** Sync docs/.../cli/install.md and packages/apm-guide/.apm/skills/apm-usage/dependencies.md with the new semver-range syntax so users discovering the CLI reference and agents reasoning over the in-product skill both teach the supported syntax. 4. **[oss-growth-hacker, recommended]** Add CHANGELOG [Unreleased] > Added entry so the next release narrative has raw material for the 'no more manual re-pinning of git deps' story angle. Deferred to follow-ups (out of scope for the blocking fix): - test-coverage-expert integration-with-fixtures test (needs local bare-repo fixture infrastructure -- separate effort). - supply-chain SHA format validation on lockfile replay (defense-in-depth, not exploitable today). - Remaining nits from python-architect, cli-logging, doc-writer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Follow-ups from the apm-review-panel pass have landed. Summary:
Deferred (with rationale):
Regression-trap evidence (mutation-break gate):
Lint contract: CI: all required checks pass on commit 9c709e1 -- Lint, Build & Test Shard 1/2 (Linux), Coverage Combine, PR Binary Smoke (Linux), Analyze (actions/python), CodeQL, APM Self-Check, NOTICE Drift, build, gate -- via Ready for maintainer review. |
Folds in the five Copilot review-bot threads posted late on PR #1496: 1. iter_semver_tags now substitutes {name} with the literal package_name before regex compilation, so {name}--v{version} patterns are scoped to the requested package and never accept sibling-package tags (e.g. otherpkg--v9.9.9 when resolving mypkg). Regression-trap tests added in tests/unit/deps/test_git_semver_resolver.py. 2. LockedDependency.from_dependency_ref now raises ValueError when both registry_resolution and git_semver_resolution are supplied. The two resolution paths are mutually exclusive per the docstring; the constructor previously silently combined fields from both. Test added in tests/unit/deps/test_lockfile_git_semver.py. 3. CachedDependencySource.acquire gates the lockfile-backed GitSemverResolution rebuild on ALL required fields being present (constraint, version, resolved_tag, resolved_commit). Previously only constraint was checked and missing fields were back-filled with empty strings, risking propagation of an incomplete resolution into InstalledPackage and a lockfile rewrite with empty/missing semver fields. Logic extracted into _rebuild_cached_semver_resolution helper; tests in tests/unit/install/test_cached_semver_rebuild.py. 4. Encoding rule (printable-ASCII binding) fixes: - src/apm_cli/install/phases/resolve.py:496 -- box-drawing chars (U+2500) replaced with ASCII hyphens. - docs/src/content/docs/reference/manifest-schema.md:379 -- em dash (U+2014) replaced with -- and the paragraph split for readability. - src/apm_cli/deps/lockfile.py -- em dashes (U+2014) and section signs (U+00A7) in newly added comments and docstrings replaced with ASCII equivalents (this PR's additions only; pre-existing non-ASCII in the file is left untouched). Mutation-break gate exercised for each new regression-trap test (deleted guard -> test failed -> restored guard -> test passed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot review fold-in (commit
|
| File | Site | Char(s) |
|---|---|---|
src/apm_cli/install/phases/resolve.py |
line 496 (this PR's addition) | U+2500 box-drawing -> - |
docs/src/content/docs/reference/manifest-schema.md |
line 379 paragraph | U+2014 em dash -> -- + paragraph split |
src/apm_cli/deps/lockfile.py |
line 61 comment | U+2014 em dash -> -- |
src/apm_cli/deps/lockfile.py |
add_dependency docstring (line 399) |
U+00A7 section sign -> section |
src/apm_cli/deps/lockfile.py |
_needs_v2 docstring (lines 426-429) |
U+2014 em dash + U+00A7 section sign -> ASCII |
Verified: git diff origin/main..HEAD -- src/ docs/ | (added-lines only) | non-ASCII grep is empty.
Validation evidence
uv run --extra dev ruff check src/ tests/-- All checks passeduv run --extra dev ruff format --check src/ tests/-- 1107 files already formatteduv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/-- 10.00/10bash scripts/lint-auth-signals.sh-- auth-signal lint cleanpytest tests/unit/deps tests/unit/install tests/integration -k 'semver or git or lockfile or resolve'-- 2732 passed, 135 skipped- CI on
c2a84809: green (run 26508098673)
Mutation-break gate exercised for each new regression-trap test: removing the guard caused the new test(s) to fail; restoring the guard restored green.
Ready to merge.
Adds 8 pipeline-level tests for the apm install -> RefResolver -> tag pick -> lockfile -> replay flow introduced by #1496. Patches the network seams (RefResolver.list_remote_refs, GitHubPackageDownloader.download_package) and drives the real CLI through CliRunner, asserting against the real apm.lock.yaml produced on disk. Promises covered: - A: caret range picks the highest matching tag from ls-remote output. - B: lockfile records constraint / resolved_tag / version / resolved_commit / resolved_at for every semver dep. - C: lockfile replay is offline -- no second ls-remote on re-install. - D: both default tag patterns (v{version}, {name}--v{version}) plus the bare {version} fallback are honoured. - F: drift branch -- when the install path is gone and the manifest constraint diverges from the lockfile, the resolver re-runs. - G: literal ref (v1.2.3) bypasses the semver resolver entirely; no constraint / resolved_tag fields are written. - H: GITHUB_APM_PAT is threaded into RefResolver via AuthResolver. - I: unsatisfied constraint surfaces the failed dep, the constraint, the repo, and at least one tag considered. Mutation-break gate: each promise was verified by temporarily inverting its production guard and confirming the matching test fails. Soft bugs surfaced (out of scope for this PR, flagged in the PR discussion): - apm install --update is a silent no-op for semver re-resolution when the dep's install path already exists -- the download_callback returns before _maybe_resolve_git_semver runs. Re-resolution today requires the install path to be missing. - apm install exits 0 even when 'Installation failed with N error(s)' is reported; Promise I therefore asserts on the diagnostic content, not the exit code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
E2E coverage addendum (commit c3f3642)Adds The existing suite (40 tests in Coverage matrix
Each promise was mutation-tested locally: the production guard was inverted, the matching test was confirmed to fail, the guard was restored. Soft bugs surfaced (out of scope; flagging for follow-up)
Both behaviours predate this PR; surfacing here so they don't get swallowed by the new schema work. |
Folds two follow-up bugs surfaced in the PR #1496 e2e wave: Bug 1 (#1496): `apm install --update` was a silent no-op for direct git-source semver dependencies whose install path already existed. The BFS resolver short-circuits at `install_path.exists()` and never invokes the download callback, so `_maybe_resolve_git_semver` never re-ran `git ls-remote` and the lockfile kept the previously-resolved tag. Fix: pre-purge the on-disk install path for direct git-semver deps when `update_refs` is set, forcing the resolver back through the callback. Matches npm / cargo / bundler semantics where `--update` is the explicit re-resolve trigger and must not be swallowed by the on-disk cache. The downstream `download_package` rmtrees and re-clones if the resolved tag changes, so refetch is safe. Bug 2 (#1496) BREAKING: `apm install` exited 0 even after printing "Installation failed with N error(s)". CI scripts relying on the exit code to detect failure silently passed. Fix: hard-fail (sys.exit(1)) from `render_post_install_summary` whenever any per-dep error was reported. `--force` continues to cover critical-security overrides only; it does NOT suppress general install errors, matching npm / pip / cargo where any install error -> non-zero exit. Tests: - Adds `TestUpdateReResolvesGitSemver` regression trap for Bug 1. - Adds `TestInstallExitCodeOnReportedErrors` for Bug 2. - Upgrades `TestNoMatchingTagError` to assert exit_code != 0 (was previously commented as a pre-existing concern). - Adds three summary-level unit tests pinning the new error_count -> SystemExit(1) contract. - Folds 19 pre-existing tests whose mocks silently relied on the buggy exit-0 behaviour: configures their MagicMock diagnostics with error_count=0, or stubs the downloader return so the lockfile writer can serialize it (root cause of incidental errors that the old contract swallowed). Mutation-break verified: stashing the resolve.py fix flips Bug 1's test red; stashing summary.py flips 5 Bug 2 tests red. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Folded from APM Review Panel doc-writer blocking findings on PR #1496: - install.md: both Exit codes tables and both --force Notes now state that any reported install error exits 1 and --force does NOT suppress general install errors (matches npm/pip/cargo). - install.md: --force row in Options table tightened to the same. - CHANGELOG.md: added [Unreleased] ### Changed entry marking the exit-code flip as BREAKING; added ### Fixed entry for the Bug 1 --update silent-no-op regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 2 | Well-composed git-semver resolver with clean separation; two minor architectural suggestions around dual-guard redundancy and the known-keys allowlist. |
| CLI Logging Expert | 0 | 0 | 2 | Output paths well-structured; Bug 2 exit-code fix correct; symbol usage consistent with house style. Two minor message-clarity nits. |
| DevX UX Expert | 0 | 2 | 2 | Strong alignment with npm/pip/cargo mental models. Two CHANGELOG / cli-commands.md gaps; two polish ideas. |
| Supply Chain Security | 0 | 1 | 2 | Sound integrity model (SHA pin + offline replay). No blocking security issues; one belt-and-suspenders + two diagnostics polish items. |
| OSS Growth Hacker | 0 | 2 | 2 | Strong adoption-hook feature. Two amplification gaps: README hero, CHANGELOG story-shaped beats. |
| Auth Expert | 0 | 0 | 0 | Auth-neutral. GitSemverResolver composes RefResolver cleanly; no AuthResolver bypass, no token leakage. |
| Doc Writer | 0 (was 2, folded) | 1 | 2 | All four blocking-flagged items folded into commit 3898357d. Two CHANGELOG/install.md edits landed in-PR; lockfile spec polish + duplicate sections noted for follow-up. |
| Test Coverage Expert | 0 | 0 | 0 | All critical-promise surfaces covered at integration-with-fixtures tier; bug regression-traps pass; ship. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [OSS Growth Hacker] Add README hero example showcasing semver-range syntax (
#^1.2.0next to the existing#v1.0.0line) -- top-of-funnel conversion: instantly signals "this works like npm" to drive-by visitors. - [Python Architect] Resolve dual-guard redundancy in
resolve.py: removesuppress(Exception)from the purge helper so the in-callback guard becomes a true safety net, or drop the pre-purge -- neither branch is wrong but the redundancy will confuse future contributors. - [Python Architect] Derive
LockedDependency._known_keysprogrammatically fromdataclasses.fields(cls)instead of the 38-entry hand-maintained set -- eliminates a manual-sync hazard for forward-compat carrier semantics. - [Supply Chain Security] Replace
robust_rmtreewithsafe_rmtreein the purge path for locally-visible containment guarantee -- defense-in-depth even though upstreamensure_path_withinalready prevents traversal. - [DevX UX Expert] Mirror the install.md updates into
docs/src/content/docs/reference/cli-commands.mdif that page exists as the canonical CLI reference, and consider a one-line aggregate--updatesummary at normal verbosity (Re-resolving N git-semver dependencies).
Architecture
classDiagram
direction LR
class DependencyReference {
<<ValueObject>>
+repo_url str
+reference str
+ref_kind str property
+get_unique_key() str
+get_install_path(dir) Path
}
class GitSemverResolver {
<<Strategy>>
-_ref_resolver RefResolver
-_include_prerelease bool
+resolve(owner_repo, package_name, constraint) GitSemverResolution
}
class GitSemverResolution {
<<ValueObject>>
+constraint str
+resolved_version str
+resolved_tag str
+resolved_sha str
+matched_pattern str
+resolved_at str
}
class NoMatchingTagError {
<<Exception>>
+summary str
+hint str
}
class RefResolver {
+list_remote_refs(owner_repo) list~RemoteRef~
}
class SemVer {
<<ValueObject>>
+major int
+minor int
+patch int
}
class LockedDependency {
+constraint str
+resolved_tag str
+resolved_at str
+_unknown_fields dict
+from_dict(data) LockedDependency
+to_dict() dict
}
class InstalledPackage {
+dep_ref DependencyReference
+resolved_commit str
+git_semver_resolution GitSemverResolution
}
class InstallContext {
+git_semver_resolutions dict
+installed_packages list
}
class APMDependencyResolver {
+resolve_dependencies(apm_dir) Graph
}
GitSemverResolver *-- RefResolver : delegates transport
GitSemverResolver ..> SemVer : filters + sorts
GitSemverResolver ..> GitSemverResolution : returns
GitSemverResolver ..> NoMatchingTagError : raises
InstallContext o-- GitSemverResolution : stores per-dep
InstalledPackage o-- GitSemverResolution : carries to lockfile
LockedDependency ..> GitSemverResolution : round-trips
APMDependencyResolver ..> DependencyReference : resolves
class GitSemverResolver:::touched
class GitSemverResolution:::touched
class NoMatchingTagError:::touched
class DependencyReference:::touched
class LockedDependency:::touched
class InstalledPackage:::touched
class InstallContext:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm install (--update?)"] --> B{update_refs?}
B -- yes --> C["[FS] _purge_cached_semver_paths_for_update<br/>src/apm_cli/install/phases/resolve.py"]
C --> D[APMDependencyResolver.resolve_dependencies]
B -- no --> D
D --> E["download_callback(dep_ref, modules_dir)"]
E --> F{install_path.exists?}
F -- yes --> G{ref_kind==semver AND update_refs?}
G -- no --> H[return cached install_path]
G -- yes --> I[fall through to resolve]
F -- no --> I
I --> J{dep_ref.ref_kind}
J -- literal/None --> K[skip semver resolution]
J -- semver --> L["_maybe_resolve_git_semver"]
L --> M{lockfile has matching constraint?}
M -- yes, no --update --> N[replay locked resolution -- no network]
M -- no or --update --> O["[NET] RefResolver.list_remote_refs<br/>git ls-remote"]
O --> P["GitSemverResolver.resolve"]
P --> Q{match found?}
Q -- no --> R[raise NoMatchingTagError]
Q -- yes --> S["return GitSemverResolution<br/>rewrite dep_ref.reference = resolved_tag"]
S --> T["stash on ctx.git_semver_resolutions"]
N --> T
K --> U["[NET] downloader.download_package"]
T --> U
U --> V["sources.py: build InstalledPackage"]
V --> W["lockfile.py: write apm.lock.yaml"]
W --> X["summary.py: render_post_install_summary"]
X --> Y{error_count > 0?}
Y -- yes --> Z["sys.exit(1) -- Bug 2 fix"]
Y -- no --> AA{critical_security AND NOT --force?}
AA -- yes --> AB[sys.exit 1]
AA -- no --> AC[exit 0]
R --> X
Recommendation
Code is sound and integration-tested on every critical-promise surface; the two blocking doc-drift items have been folded in commit 3898357d. Ship. The remaining follow-ups (README hero, dual-guard cleanup, safe_rmtree swap, _known_keys derivation, cli-commands.md sync) are non-blocking and can land post-merge without user-facing risk.
Full per-persona findings
Python Architect
- [recommended] Dual pre-purge AND callback guard creates a redundant code path at
src/apm_cli/install/phases/resolve.py:148-220
The--updatepath has two mechanisms keeping the install-path from short-circuiting: pre-purge AND in-callback guard. If pre-purge succeeds, the callback guard at line 438 is dead code. Two overlapping mechanisms for the same invariant increases maintenance cost.
Suggested: Removesuppress(Exception)from the purge helper and let it log a warning on failure -- then the in-callback guard becomes a true safety net rather than routinely-unreachable code. Alternatively, drop the pre-purge entirely and rely solely on the callback guard. - [recommended]
_known_keysallowlist inlockfile.py from_dictis fragile atsrc/apm_cli/deps/lockfile.py:169-207
Hand-maintained 38-entry set must stay in sync with@dataclassfield declarations. If a future contributor adds a field but forgets_known_keys, that field silently ends up in_unknown_fieldsand round-trips incorrectly.
Suggested:_known_keys = {f.name for f in fields(cls) if not f.name.startswith('_')} | {'deployed_skills'}. - [nit]
_maybe_resolve_git_semvermixes guard logic and I/O atsrc/apm_cli/install/phases/resolve.py:64-155-- extract the lockfile-replay branch into a pure_replay_locked_semver(dep_ref, existing_lockfile)for testability. - [nit]
iter_semver_tagsreturns a list but the name implies a generator atsrc/apm_cli/deps/git_semver_resolver.py:157-175-- rename tocollect_semver_tagsor convert to a true generator.
CLI Logging Expert
- [nit]
verbose_detailmessage includes redundant bracket symbol atsrc/apm_cli/install/phases/resolve.py:~965-- message reads like internal commentary; trim to scan-readable format like[*] --update: purged {key} (semver re-resolve). - [nit]
NoMatchingTagErrorfail_msgduplicates exception text viastr(e)atsrc/apm_cli/install/phases/resolve.py:~1045-- repo name appears twice in the same line.
DevX UX Expert
- [recommended] CHANGELOG.md missing entry for exit-code breaking change (Bug 2) -- RESOLVED: folded into commit
3898357d. - [recommended]
docs/src/content/docs/reference/cli-commands.mdnot updated for semver-rangeref:syntax. Per project convention "if a CLI change is not reflected incli-commands.mdin the same PR, that change is incomplete by definition." - [nit]
--updatecould emit a one-line summary at normal verbosity (Re-resolving N git-semver dependencies) aggregated, not per-dep. - [nit]
NoMatchingTagErrorhint could interpolate the highest-found version for one-copy-paste recovery (pin a literal tag with ref: v1.4.2 instead of ^1.2.0).
Supply Chain Security Expert
- [recommended] Purge uses
robust_rmtreeinstead ofsafe_rmtreeatsrc/apm_cli/install/phases/resolve.py:180-192-- not exploitable (path validated upstream byget_install_path->ensure_path_within), butsafe_rmtreewould make the containment guarantee locally visible and guard against future refactors. Defense-in-depth. - [nit] Auth lookup swallows all exceptions silently at
src/apm_cli/install/phases/resolve.py:142-148-- bareexcept Exception: token = Nonemakes auth misconfiguration invisible. Log at verbose/debug level. - [nit]
NoMatchingTagErrorexposes tag names from the remote atsrc/apm_cli/deps/git_semver_resolver.py:266-272-- fine for public repos; leaks release naming for private/enterprise.
OSS Growth Hacker
- [recommended] README.md hero should showcase semver range syntax at
README.md:35-- currently shows only exact tag pinning; adding#^1.2.0instantly signals "this works like npm" at the top-of-funnel conversion surface. - [recommended] CHANGELOG.md missing
### Fixedsection for both bug fixes -- PARTIALLY RESOLVED: Bug 1 (--updateno-op) entry landed in commit3898357d; Bug 2 entry landed in### Changedwith BREAKING marker. - [nit]
manage-dependencies.md: consider leading with npm familiarity hook (If you use ^1.2.0 in package.json or ~=1.4 in requirements.txt, the same idea works here). - [nit] apm-guide skill update is good for agent-mediated discovery (an underrated growth vector).
Auth Expert
No findings. GitSemverResolver composes RefResolver (already uses build_https_clone_url with explicit token param and redacts tokens from error output). resolve.py wiring calls auth_resolver.resolve_for_dep(dep_ref) to obtain per-dep AuthContext before constructing RefResolver -- matching the same credential path the downstream clone uses. No new clone-URL construction, no direct os.getenv() for tokens, no host-classification changes, no token leakage.
Doc Writer
- [recommended] (was blocking, RESOLVED) install.md exit-code table and
--forcenote now reflect Bug 2 BREAKING flip -- both duplicate Exit codes tables and--forceNotes updated in commit3898357d. - [recommended] (was blocking, RESOLVED) CHANGELOG BREAKING entry for exit-code flip landed in commit
3898357dunder### Changed. - [recommended] CHANGELOG
### Fixedentry for Bug 1 (--updategit-semver re-resolution) -- RESOLVED: landed in commit3898357d. - [nit] Lockfile spec: clarify that
versionandresolved_commitare also pinned for git-semver -- single-sentence orientation note above/below the new rows. - [nit] Two duplicate
## Exit codesand## Notessections ininstall.md(pre-existing structural duplication, not introduced by this PR). When patching Bug 2 fix, both were updated in lockstep (verified). Out-of-scope to consolidate here.
Test Coverage Expert
No findings. All critical-promise surfaces covered at integration-with-fixtures tier; all probed tests pass on this commit. Evidence (all passed):
- Install pipeline (
--updatere-resolve) --tests/integration/test_git_semver_install_e2e.py::TestUpdateReResolvesGitSemver-- integration-with-fixtures --1 passed in 4.19s-- provesapm install --updatere-resolves git-semver constraints against latest remote tags even when install path exists on disk [DevX, Portability] - Install pipeline (exit-code contract) --
tests/integration/test_git_semver_install_e2e.py::TestInstallExitCodeOnReportedErrors-- integration-with-fixtures --1 passed in 1.85s-- provesapm installexits non-zero wheneverInstallation failed with N error(s)is reported [DevX, Secure by default] - Exit-code unit tier --
tests/unit/install/test_summary.py(3 cases:test_hard_fail_on_reported_errors_without_critical_security,test_force_does_not_suppress_reported_errors,test_error_count_forwarded_to_install_summary) -- unit --3 passed in 1.32s-- proveserror_count > 0triggerssys.exit(1);--forcedoes NOT suppress reported-error hard-fail [DevX] - Lockfile determinism --
tests/integration/test_git_semver_install_e2e.py::TestLockfileReplayIsOffline-- integration-with-fixtures --1 passed in 3.27s-- proves second install with unchanged manifest replays from lockfile without network calls [Portability, DevX] - Auth threading --
tests/integration/test_git_semver_install_e2e.py::TestAuthTokenThreadedToLsRemote-- integration-with-fixtures --1 passed in 1.97s-- provesGITHUB_APM_PATis threaded intoRefResolverforgit ls-remoteon semver deps [Secure by default, DevX] - Cached semver rebuild gate --
tests/unit/install/test_cached_semver_rebuild.py(8 cases) -- unit --8 passed in 0.53s-- proves incomplete lockfile entries do not produce half-populatedGitSemverResolutionobjects [Secure by default, Governed by policy]
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Closes #1488.
TL;DR
Adds a
GitSemverResolverso git-source deps inapm.ymlcan carry asemver range as their
ref:(e.g.acme/widget#^1.2.0). At installtime APM lists the remote's tags, picks the highest one satisfying the
range, and pins the concrete tag + commit + version in the lockfile.
Why
Today,
ref:accepts only literal git refs (main,v1.5.3, a SHA).Consumers who want "any 1.x >= 1.2.0" must either hand-pick a tag and
re-pin manually on each release, or vendor a registry. This blocks
git-source deps from offering the same npm-style ergonomics the
dedicated-registry path already enjoys.
How
src/apm_cli/deps/git_semver_resolver.py—composition over the existing
RefResolver+marketplace.semver+marketplace.tag_patternprimitives. Two-pass: primary patterns(
v{version},{name}--v{version}, matching PR feat(deps): support marketplace dependencies in plugin.json #1422 convention),then bare
{version}fallback. RaisesNoMatchingTagErrorwith theinspected pattern list when nothing matches.
ref_kindproperty onDependencyReference—classifies
ref:as"semver"/"literal"/None.install/phases/resolve.py:_maybe_resolve_git_semver— runs in the BFS
download_callbackBEFORE clone, stashes theresolution on
ctx.git_semver_resolutions, and rewritesdep_ref.referenceto the concrete tag so the rest of the pipeline(drift / download / hash) operates on a literal ref.
constraint,resolved_tag,resolved_at. Forward-compat allowlist infrom_dict/to_dictpreserves unknown future keys when older APM rewrites the lockfile.
locked constraint, the resolved tag is replayed without a network
call.
apm install --updateor a manifest change re-resolves.detect_ref_changenow treats a semver-rangemanifest ref vs. a literal locked tag as 'no drift' when the locked
constraint equals the manifest range — mirrors the existing
_registry_range_covers_locked_versionpath for registry deps.Validation
install/drift wiring). All pass.
ruff check, ruff format --check, pylint R0801, auth-signal lint —
all clean.
How to test
Re-running
apm installis a no-op (lockfile replay).apm install --updatere-resolves against current remote tags.Trade-offs
guarded by a
from_dictallowlist +_unknown_fieldscarrier soolder APM readers preserve unknown keys instead of dropping them on
re-emit. No v3 bump needed.
marketplace.semver+marketplace.tag_patternmodules are reusedin place rather than refactored into a shared base — keeping the
blast radius small for this PR. R0801 clean at 10-line threshold.
Bugs fixed in this PR (e2e wave follow-ups)
The e2e wave (commit c3f3642) surfaced two latent bugs that this PR
now also fixes:
Bug 1 —
apm install --updatesilent no-op for git-semverWhen a direct git-source semver dep's install path already existed,
the BFS resolver short-circuited at
install_path.exists()and neverinvoked
download_callback, so_maybe_resolve_git_semverneverre-ran
git ls-remote. The lockfile kept the previously-resolved tageven though the user explicitly asked for
--update.Fix: pre-purge the on-disk install path for direct git-semver deps
when
update_refsis set, forcing the resolver back through thecallback. Matches npm / cargo / bundler:
--updateis the explicitre-resolve trigger and must not be swallowed by the on-disk cache.
Scoped to direct deps; transitive deps re-walk naturally once a direct
dep's callback rewrites its ref. Local, registry, and proxy deps are
excluded.
Regression trap:
TestUpdateReResolvesGitSemverintests/integration/test_git_semver_install_e2e.py.Bug 2 —
apm installexited 0 on reported errors (BREAKING)apm installexited 0 even after printingInstallation failed with N error(s), so CI scripts could not detect failure via exit code.Fix:
render_post_install_summarynowsys.exit(1)whenevererror_count > 0.--forcecontinues to cover critical-securityoverrides only; it does NOT suppress general install errors. Matches
npm / pip / cargo: any install error -> non-zero exit, no override.
Regression traps:
TestInstallExitCodeOnReportedErrorsand thehardened
TestNoMatchingTagErrorintest_git_semver_install_e2e.py,plus three new summary-level unit tests in
tests/unit/install/ test_summary.py.BREAKING for callers asserting exit_code == 0 when errors were
reported. Folded in this PR: 19 pre-existing tests across 8 files
relied on the silent-failure pattern; they are now corrected (either
by completing their MagicMock so no incidental diagnostic fires, or by
configuring
error_count=0on the test mock). No production callersin this repo depend on the old behaviour.
Mutation-break gate
resolve.pypre-purge flipsTestUpdateReResolvesGit SemverRED. Restoring -> GREEN.summary.pyexit-code branch flips 5 Bug-2 tests RED.Restoring -> GREEN.