feat(registry): align object-form registry deps with git skills:#2166
feat(registry): align object-form registry deps with git skills:#2166nadav-y wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Aligns registry object-form dependency parsing/serialization with the existing git object-form behavior by allowing a skills: subset and ensuring registry deps round-trip correctly through to_apm_yml_entry(). This improves manifest fidelity when users install a subset of skills from a registry-hosted SKILL_BUNDLE and then persist it back to apm.yml.
Changes:
- Accept
skills:on registry longhand (id/registry/version) entries by reusing the sharedparse_skill_subsetvalidator. - Add a dedicated
source == "registry"branch inDependencyReference.to_apm_yml_entry()so registry deps serialize as registry dict entries rather than malformed git entries. - Add unit tests covering registry
skills:parsing, validation, and round-trip serialization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/unit/test_reference_registry_shorthand.py | Adds unit tests for registry object-form skills: parsing/validation and to_apm_yml_entry() round-trips. |
| src/apm_cli/models/dependency/reference.py | Adds skills: support to registry object-form parsing and adds a registry serialization branch in to_apm_yml_entry(). |
Copilot review on microsoft#2166 correctly flagged that the new source == "registry" branch in to_apm_yml_entry() dropped target_subset: set_target_subset_for_entry() sets ref.target_subset directly and calls to_apm_yml_entry(), so a registry dependency's targets: write-back was silently lost. Since the PR's premise is full parity with git-longhand (which already supports both skills: and targets:), close the gap by adding targets: parsing/validation to the registry object-form parser (same parse_target_subset validator as git/local-path forms) and emitting it in to_apm_yml_entry(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Fixed the file-length CI failure: extracted |
|
Pushed Root cause: Fix:
Verified live against the real registry: the exact repro command now correctly leaves apm.yml as Also confirmed via a separate manual test: the other finding ( |
|
Fixed the Spec conformance gate / Mode B detector failure: my previous commit's fix in Since this is a genuine bug fix (not a no-op refactor), I did the proper three-step ritual rather than reaching for
Verified locally: |
…lls: Object-form git dependencies (- git: ...) have long supported a skills: array to install a subset of a SKILL_BUNDLE package. The registry-longhand form (- id: owner/repo, registry:, version:) rejected the same field as unknown, with no way to narrow which skills get deployed. Reuses the existing parse_skill_subset validator so both forms share identical validation (non-empty, no path traversal, sorted+deduped). Also fixes to_apm_yml_entry(), which had no branch for source == "registry" at all: any registry dependency with a skill/target subset set (e.g. via apm install --skill ... --save) would previously serialize as a malformed git: entry, and even an alias-only registry entry collapsed to a bare canonical string, silently dropping registry:/id:/version:. Verified end-to-end against a live registry: published a package with 4 skills, installed a consumer with skills: [a, b], confirmed only 2 were deployed and apm.lock.yaml's skill_subset matched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot review on microsoft#2166 correctly flagged that the new source == "registry" branch in to_apm_yml_entry() dropped target_subset: set_target_subset_for_entry() sets ref.target_subset directly and calls to_apm_yml_entry(), so a registry dependency's targets: write-back was silently lost. Since the PR's premise is full parity with git-longhand (which already supports both skills: and targets:), close the gap by adding targets: parsing/validation to the registry object-form parser (same parse_target_subset validator as git/local-path forms) and emitting it in to_apm_yml_entry(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…le-length gate CI's file-length guardrail failed: reference.py was already at 2079 lines on main (the existing worst offender driving the 2100-line cap) and this PR's additions tipped it to 2124. Move _parse_registry_object_entry into a new sibling module models/dependency/registry_entry.py, matching the existing pattern of object_fields.py/subsets.py/identity.py/types.py -- reference.py delegates via a thin classmethod wrapper. The new module takes the DependencyReference class as a parameter instead of importing it, so there's no import cycle. reference.py: 2124 -> 2028 lines. Scoped to only the registry object-form parsing this PR touches; the rest of reference.py (git/URL shorthand parsing, GitLab/ADO handling, virtual package detection) is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Repro: apm.yml already has an object-form registry dependency (`- id: owner/repo`, `version: ...`). Running `apm install "owner/repo#1.0.0" --skill some-skill` rewrote that entry into a `git:`-shaped structured entry and then failed ls-remote (or worse, silently succeeded if `owner/repo` also happened to resolve as a real GitHub repo -- a dependency-confusion-shaped footgun). Root cause: DependencyReference.parse() (the CLI positional parser) has no registry awareness -- source is always None/"git" for a bare string. The --skill code path in _resolve_package_references() calls dep_ref.to_apm_yml_entry() before the (separate) default-registry routing check runs, so the registry-aware branch in to_apm_yml_entry() never fires for a freshly-parsed CLI arg, even when apm.yml already has this exact identity pinned to a registry. Fix: - _resolve_package_references() now looks up the existing apm.yml entry for the parsed identity before doing anything else; if it's source=="registry", propagate source/registry_name onto the freshly-parsed dep_ref so to_apm_yml_entry() takes the registry branch, and route the probe-skip decision the same way (routes_unscoped_to_registry treats an already- known registry source as "not ambiguous", so the existing-registry fact has to be OR'd in explicitly). - merge_structured_entry_into_current_deps() gets a defense-in-depth guard: refuse (ValueError, caught and reported as a normal invalid-package outcome) to overwrite a registry-sourced manifest entry with a non- registry-shaped structured entry, regardless of which upstream code path produced it. - Extracted the repeated "parse current_deps entries, find the one matching this identity" loop into get_existing_dep_ref_for_identity() (used by the new check and by get_existing_skill_subset(), which had its own copy of the same loop). Verified live against a real registry: the exact repro command now leaves apm.yml as a proper id/registry/version dict with skills: added, and apm.lock.yaml resolves it as source: registry from the actual registry URL. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rewrite Spec-citation fold closing the Mode B silent-extension gate on src/apm_cli/install/package_resolution.py (the --skill registry-to-git silent-conversion fix). Rebased onto latest main, which independently landed its own req-mf-022 (closes microsoft#2116, a different but adjacent requirement: diagnosing a stale skills: subset that matches zero available skills). Renumbered this PR's citation to req-mf-023 to avoid the id collision. Added req-mf-023 (Section 4.3.2, consumer MUST): a consumer MUST NOT silently rewrite an existing id:-form (registry-sourced) manifest entry into a git:-form entry when persisting a subsequent CLI-driven update (e.g. an additive --skill pin) for the same dependency identity; the existing entry's source MUST be honored, and an update that would otherwise replace a registry-sourced entry with a non-registry-shaped entry MUST be rejected with a diagnostic naming the identity. Three-step ritual: spec anchor + Appendix C row + Section 4.9/11.3.2 enumerations + revision history row (counts: 104 -> 105, 99 -> 100 MUST); manifest entry in openapm-v0.1.requirements.yml; @pytest.mark.req ("req-mf-023") test driving the real merge_structured_entry_into_current_deps behavior. Regenerated CONFORMANCE.{md,json}. orphan_check passes (105 requirements aligned). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
959bb64 to
f3bf2ce
Compare
|
Rebased onto latest
Re-verified after rebase: full relevant suite (3302 passed, same 2 pre-existing unrelated |
Summary
- git: ...) have long supported askills:array to install a subset of skills from a SKILL_BUNDLE package. The registry-longhand form (- id: owner/repo,registry:,version:) rejected the same field as an unknown key, with no way to narrow which skills get deployed. This aligns registry-longhand with git-longhand by reusing the sameparse_skill_subsetvalidator (non-empty, no path traversal, sorted+deduped).to_apm_yml_entry(), which had no branch forsource == "registry": any registry dependency with a skill/target subset set (e.g. viaapm install --skill ... --save) previously serialized as a malformedgit:entry, and even an alias-only registry entry collapsed to a bare canonical string, silently droppingregistry:/id:/version:.Test plan
tests/unit/test_reference_registry_shorthand.py::TestObjectFormRegistrycovering: parsingskills:, empty-list rejection, path-traversal rejection, round-trip throughto_apm_yml_entry(), and that alias-only registry entries no longer collapse to a plain string.test_reference_registry_shorthand.py,test_skill_subset_persistence.py, and broader dependency/install/lockfile tests).skills: [skill-alpha, skill-beta], confirmed only 2 skills were deployed to.claude/skills/, andapm.lock.yaml'sskill_subsetmatched exactly.