Skip to content

[BUG] fix(audit): trust lockfile claude_skill type when apm_modules not materialized#2329

Open
hugoguitton-lucca wants to merge 1 commit into
microsoft:mainfrom
hugoguitton-lucca:fix/config-consistency-cold-cache-virtual-skill
Open

[BUG] fix(audit): trust lockfile claude_skill type when apm_modules not materialized#2329
hugoguitton-lucca wants to merge 1 commit into
microsoft:mainfrom
hugoguitton-lucca:fix/config-consistency-cold-cache-virtual-skill

Conversation

@hugoguitton-lucca

Copy link
Copy Markdown
Contributor

Summary

Fixes #2328.

apm audit --ci failed the config-consistency check for valid virtual claude_skill dependencies whenever it ran without a prior apm install (a setup-only CI job that installs the CLI and audits, so apm_modules/ is never materialized).

#2214 fixed the case where the modules are materialized, but its _allows_missing_manifest implementation probes the on-disk shape via detect_package_type(package_dir). With apm_modules/ absent, that returns INVALID, the exemption is denied, and the check hard-fails — even though drift in the same run gracefully skips the identical cold cache (drift skipped: install cache not populated).

Fix

Mirror the drift check's cold-cache tolerance: when the package directory is not materialized on disk, fall back to the frozen lockfile classification instead of hard-failing.

    if not package_dir.exists():
        return dependency.package_type == PackageType.CLAUDE_SKILL.value
    package_type, _ = detect_package_type(package_dir)
    return package_type is PackageType.CLAUDE_SKILL
  • When apm_modules/ is materialized, the strict on-disk shape probe still runs, so a mislabeled (non-virtual) or malformed (missing SKILL.md) installed package is still rejected.
  • Preserves the four E2E scenarios from fix(audit): exempt is_virtual packages from config-consistency manifest check #2214 (they always apm install — and apm install --frozen, which re-materializes apm_modules/ — before auditing, so package_dir exists and the guard never triggers).

Tests

Added a unit regression that reproduces the setup-only CI topology (virtual claude_skill locked, apm_modules/ not materialized) and asserts the missing manifest is waived:

tests/unit/integration/test_mcp_config_view.py::test_manifestless_virtual_skill_skipped_when_modules_not_materialized

tests/unit/integration/test_mcp_config_view.py: 18 passed (17 existing + 1 new). The existing test_manifestless_virtual_package_without_skill_shape_records_problem (modules materialized but no SKILL.md) still fails as expected, confirming the strict probe is unchanged when the directory exists.

Repro (before this PR)

- uses: microsoft/apm-action@v1
  with:
    setup-only: true
    apm-version: 0.26.0
- run: apm audit --ci   # config-consistency: "package manifest not found ... re-run 'apm install'"

…erialized

config-consistency probed the on-disk package shape to exempt manifestless
virtual Claude skills. In a setup-only CI job (install the CLI, then
'apm audit --ci' with no 'apm install'), apm_modules/ is never materialized,
so detect_package_type returns INVALID, the exemption is denied, and the check
hard-fails on valid claude_skill deps -- while the drift check gracefully
skips the same cold cache.

Fall back to the frozen lockfile classification when the package directory is
absent, mirroring drift's cold-cache tolerance. The strict on-disk shape probe
still runs once apm_modules/ is materialized, so mislabeled or malformed
installed packages are still rejected.

Fixes microsoft#2328

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes apm audit --ci failing config-consistency for valid virtual claude_skill dependencies when running in a “setup-only” CI topology (no prior apm install, so apm_modules/ is not materialized). The change keeps the strict on-disk shape probe when the package directory exists, but falls back to the lockfile’s package_type when it does not.

Changes:

  • Update _allows_missing_manifest() to tolerate a cold/absent apm_modules/ by trusting the lockfile’s claude_skill classification when the package directory is missing.
  • Add a unit regression test covering the setup-only CI scenario (virtual skill locked, install tree absent) to ensure the missing-manifest problem is waived.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/apm_cli/integration/mcp_config_view.py Allows missing apm.yml for virtual claude_skill when the install dir is absent, while preserving strict on-disk detection when present.
tests/unit/integration/test_mcp_config_view.py Adds a regression test for “no apm install / no apm_modules” to ensure config-consistency doesn’t hard-fail.

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.

[BUG] config-consistency fails for virtual claude_skill deps in setup-only CI (no apm install)

2 participants