fix(integration): tolerate cold-cache apm_package deps in MCP frozen check - #2457
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where apm install --frozen could fail on a cold cache when MCP state is present, by teaching the MCP config derivation to tolerate lock-pinned remote dependencies whose on-disk install directory has not been materialized yet.
Changes:
- Generalize
_allows_missing_manifest()to treat a missing package directory as cold-cache state for non-local dependencies (instead of reporting “manifest not found”). - Preserve failure behavior for genuinely suspicious states (directory present but
apm.ymlmissing) and for missing local dependency directories. - Add targeted unit tests covering the cold-cache remote
apm_packagecase and the two negative controls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/apm_cli/integration/mcp_config_view.py |
Broadens cold-cache tolerance for missing dependency install directories during MCP config derivation. |
tests/unit/integration/test_mcp_config_view.py |
Adds regression and control tests for cold-cache tolerance vs corruption/local-missing cases. |
| if not package_dir.exists(): | ||
| return dependency.source != "local" |
There was a problem hiding this comment.
Good catch, real gap. Fixed in d2e10e0: _package_manifest_path now checks is_symlink() on the unresolved package_dir before calling .resolve() (which would otherwise silently follow the symlink -- including a broken one -- and erase the signal), and threads that boolean explicitly into _allows_missing_manifest so a dangling apm_modules/<dep> entry still fails as a genuine problem instead of being waived as cold-cache state. Added test_dangling_symlink_apm_package_still_records_problem as a regression proof.
f1bae46 to
fde0358
Compare
Addresses Copilot review feedback on microsoft#2457: Path.exists() follows symlinks and returns False for a broken one, making it indistinguishable from a directory that was simply never fetched. A dangling apm_modules/<dep> entry is a present, corrupted install, not cold-cache state, and must still fail. _package_manifest_path now checks is_symlink() on the unresolved package_dir before calling .resolve() (which would otherwise silently substitute the symlink's target and erase the signal), and threads that boolean into _allows_missing_manifest explicitly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sergio Sisternes (sergio-sisternes-epam)
added
the
panel-review
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/apm_cli/integration/mcp_config_view.py:213
_package_manifest_pathintends to capturepackage_dir.is_symlink()before calling.resolve()(docstring explicitly depends on that), but the currentreturn (package_dir / "apm.yml").resolve(), package_dir.is_symlink()evaluates left-to-right, so.resolve()runs first. For a symlinked (including broken) package dir, this can follow the symlink and lose the original-path signal you are trying to preserve.
if dependency.source == "local":
package_dir = resolve_local_dep_dir(dependency, lockfile, project_root)
else:
package_dir = dependency.to_dependency_ref().get_install_path(modules_root)
return (package_dir / "apm.yml").resolve(), package_dir.is_symlink()
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | Clean, minimal fix with correct single-owner discipline; symlink flag threading is sound; test coverage is excellent. |
| CLI Logging Expert | 0 | 0 | 1 | No CLI output regression; cold-cache path is correctly silent -- verbose-mode breadcrumb would be nice but non-blocking. |
| DevX UX Expert | 0 | 0 | 0 | Fix correctly aligns --frozen cold-cache behavior with user expectations; no UX regressions. |
| Supply Chain Security Expert | 0 | 0 | 0 | Cold-cache waiver is safe: it only fires when the directory is entirely absent (not a symlink), the lockfile pin remains the integrity gate, and no code execution or content trust decision is made at this point. |
| OSS Growth Hacker | 0 | 0 | 2 | High-value CI friction fix with an excellent CHANGELOG entry; no growth blockers. |
| Doc Writer | 0 | 1 | 2 | CHANGELOG entry is accurate and voice-consistent; one recommended clarification to the Frozen mode note in reference/cli/install.md to surface the cold-cache carve-out. |
| Test Coverage Expert | 0 | 1 | 1 | Four well-targeted unit tests cover every branch of the fix; integration-tier test for cold-cache + frozen MCP path is missing but unit coverage is adequate for a bug-fix PR. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Doc Writer] Document the cold-cache carve-out in the Frozen mode note in reference/cli/install.md -- Users searching docs after a fresh-checkout CI failure will not find an explanation of the tolerance behavior without this addition.
- [Test Coverage Expert] Add integration-with-fixtures test exercising apm install --frozen on a cold cache with a remote apm_package dep -- Missing integration-tier test on a devx-promise surface; unit tests cover the branches but an end-to-end assertion (exit-code 0, no false-positive error) would survive future refactors of the install pipeline.
- [CLI Logging Expert] Add verbose-mode breadcrumb when cold-cache tolerance skips a dependency -- Helps --verbose users and AI agents understand why a locked dep contributed zero MCP servers; low-cost observability improvement.
- [OSS Growth Hacker] Include the error message symptom ('manifest not found') in the CHANGELOG entry for grep discoverability -- Users searching CI logs will Ctrl-F the error they saw; including it in the entry text improves release-note discoverability.
- [Python Architect] Consider a ManifestLookup dataclass instead of bare tuple return from _package_manifest_path -- Self-documenting and extensible; the boolean's meaning is opaque at call sites. Polish-tier, not urgent.
Architecture
classDiagram
direction LR
class LockedDependency {
<<ValueObject>>
+source str
+package_type str
+to_dependency_ref() DependencyRef
}
class DependencyRef {
<<ValueObject>>
+is_virtual_subdirectory() bool
+get_install_path(root) Path
}
class LockFile {
<<ValueObject>>
+dependencies list
}
class mcp_config_view {
<<Module>>
+_package_manifest_path(dep, lf, mr, pr) tuple
+_allows_missing_manifest(dep, dir, is_symlink) bool
+_collect_locked_dependencies(...) list
}
class PackageType {
<<Enum>>
SKILL_BUNDLE
CLAUDE_SKILL
APM_PACKAGE
}
class detect_package_type {
<<Pure>>
+(dir) tuple
}
mcp_config_view ..> LockedDependency : reads
mcp_config_view ..> LockFile : reads
mcp_config_view ..> DependencyRef : calls
mcp_config_view ..> PackageType : compares
mcp_config_view ..> detect_package_type : calls
LockedDependency ..> DependencyRef : creates
class mcp_config_view:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["_collect_locked_dependencies\nsrc/apm_cli/integration/mcp_config_view.py"] --> B["_package_manifest_path\nresolve path + is_symlink flag"]
B --> C{"manifest_path.exists()?"}
C -->|Yes| D["Parse manifest, collect MCP state"]
C -->|No| E["_allows_missing_manifest"]
E --> F{"package_type == SKILL_BUNDLE?"}
F -->|Yes| G["skip -- tolerated"]
F -->|No| H{"dir missing AND\nnot symlink?"}
H -->|Yes| I{"source != local?"}
I -->|Yes| J["skip -- cold cache tolerated"]
I -->|No| K["record problem\n[FS] missing local dep"]
H -->|No| L{"is_virtual_subdirectory?"}
L -->|No| M["record problem\n[FS] missing manifest"]
L -->|Yes| N{"detect_package_type == CLAUDE_SKILL?"}
N -->|Yes| G
N -->|No| M
Recommendation
All specialists agree this is a correct, minimal, security-safe bug fix with solid unit coverage. The doc gap and missing integration test are real but neither blocks shipping -- they are follow-up work that can land in a subsequent PR. The fix resolves a user-facing CI false-positive, is community-contributed, and reinforces APM's CI reliability positioning. Ship promptly to signal contributor-friendly velocity.
Full per-persona findings
Python Architect
-
[nit] Consider a small dataclass instead of a bare tuple return from _package_manifest_path at
src/apm_cli/integration/mcp_config_view.py:195
Returning tuple[Path, bool] works but the boolean's meaning is opaque at call sites. A frozen dataclass ManifestLookup(path: Path, is_symlink: bool) would be self-documenting and extensible. At current scope this is polish, not blocking.
Suggested: Introduce DataClass(frozen=True) class ManifestLookup: path: Path; is_symlink: bool and return that instead of a raw tuple. -
[nit] Guard order in _allows_missing_manifest could benefit from a one-line comment on precedence at
src/apm_cli/integration/mcp_config_view.py:222
The new cold-cache guard sits between the skill_bundle early-return and the virtual-subdirectory check. The ordering is correct but a one-line comment on why cold-cache must precede detect_package_type (which would crash on missing dir) would help future readers.
CLI Logging Expert
- [nit] New cold-cache tolerance branch is silent in verbose mode at
src/apm_cli/integration/mcp_config_view.py
The new early-return in _allows_missing_manifest skips the dependency with no trace in verbose mode. Adding a logger.verbose_detail call before the continue would help --verbose users and AI agents understand why a locked dep contributed zero MCP servers.
Suggested: Addif logger: logger.verbose_detail(f"Cold cache: skipping '{package_key}' -- package not yet fetched")beforecontinuein the cold-cache block in _collect_locked_dependencies.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
-
[nit] CHANGELOG entry could include the error message symptom for grep discoverability at
CHANGELOG.md
Users searching their CI logs will grep for the error message they saw. Including 'manifest not found' in the entry text would make the CHANGELOG entry discoverable via Ctrl-F in release notes. -
[nit] Release-note story angle: CI-first credibility beat for next release post
This fix reinforces APM's 'just works in CI' positioning. Fresh-checkout correctness is a trust signal for platform teams evaluating APM. Recommend leading next release post with 'CI reliability' theme.
Auth Expert -- inactive
PR touches only mcp_config_view.py (manifest path resolution) and its test -- no auth, token, or credential resolution surface affected.
Doc Writer
-
[recommended] Frozen mode note in docs does not mention the cold-cache apm_package carve-out at
docs/src/content/docs/reference/cli/install.md:122
docs/src/content/docs/reference/cli/install.md line 122 (Frozen mode note) states 'MCP config state that differs from apm.yml exits 1' but does not mention that an absent, not-yet-fetched apm_package directory (cold cache) is tolerated. A user searching docs after a fresh-checkout CI failure won't find this carve-out documented. Verified the text at line 122.
Suggested: Append to the Frozen mode note: '; a not-yet-fetched git-hosted apm_package directory (cold cache) is tolerated and is not treated as MCP state drift' -
[nit] CHANGELOG uses 'lockfile drift' where 'MCP state drift' is more precise at
CHANGELOG.md
The term 'lockfile drift' has a precise meaning in APM docs (apm.yml vs apm.lock.yaml disagreement). This fix addresses MCP frozen check false-positive, not lockfile drift. Using 'MCP state drift' or 'drift' avoids semantic confusion.
Suggested: Replace 'lockfile drift' with 'MCP state drift' or simply 'drift' in the CHANGELOG entry. -
[nit] CHANGELOG entry uses 'Reported by Rafael Azevedo (@rrazvd)' attribution not present in adjacent entries at
CHANGELOG.md
Style divergence from surrounding entries; not an error but inconsistent with the current CHANGELOG voice. No action required unless a uniform attribution policy is adopted.
Test Coverage Expert
-
[recommended] No integration-with-fixtures test exercises cold-cache tolerance in the frozen MCP check end-to-end at
tests/integration/test_mcp_only_lockfile_lifecycle.py
The tier floor for install-pipeline behavior is integration-with-fixtures. The 4 new unit tests exercise _allows_missing_manifest directly via CurrentMcpConfigView.derive(), but no integration test runsapm install --frozenwith a missing apm_modules/ for a remote apm_package dependency and asserts exit-code 0.
Proof (missing):tests/integration/test_mcp_only_lockfile_lifecycle.py::test_frozen_cold_cache_remote_apm_package_passes_without_modules-- proves: apm install --frozen succeeds on a fresh clone where apm_modules/ was never materialised for a remote apm_package dependency [devx]
assert frozen.returncode == 0; assert 'manifest not found' not in frozen.stdout -
[nit] Unit tests adequately cover all 4 behavioral branches of the fix at the unit tier at
tests/unit/integration/test_mcp_config_view.py
test_cold_cache_nonvirtual_apm_package_is_tolerated, test_materialized_but_manifestless_apm_package_still_records_problem, test_cold_cache_local_dependency_still_records_problem, and test_dangling_symlink_apm_package_still_records_problem each assert the correct problem count and message shape.
Proof (passed):tests/unit/integration/test_mcp_config_view.py::test_cold_cache_nonvirtual_apm_package_is_tolerated-- proves: Absent apm_modules directory for a non-local apm_package dep is tolerated (no problems recorded) [devx]
assert not package_dir.exists(); assert view.problems == ()
Performance Expert -- inactive
PR touches only mcp_config_view.py (integration layer) and its test; no cache, transport, resolve, or materialization path is affected.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Sergio Sisternes (sergio-sisternes-epam)
removed
the
panel-review
…check (closes microsoft#2456) _allows_missing_manifest only waived an absent apm.yml for skill_bundle packages, or a virtual-subdirectory dependency whose materialized shape detects as claude_skill. A plain, non-virtual git apm_package dependency (repo root ships its own apm.yml) never matched either branch, so on a cold cache -- apm_modules/ never materialized, as on a fresh CI checkout -- CurrentMcpConfigView.derive() reported "package manifest not found", and any project combining that shape with MCP state failed `apm install --frozen` even though the lockfile pin was intact and a plain install would restore it without any lockfile change. Generalize the cold-cache tolerance: when the package's entire install directory doesn't exist at all (not just its apm.yml), a non-local dependency simply hasn't been fetched yet, regardless of package_type -- mirroring the microsoft#2329 tolerance apm audit --ci already has for the same state. A present-but-manifestless directory (genuine corruption) and a missing local dependency (not cold-cache-explainable) still report a problem. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses Copilot review feedback on microsoft#2457: Path.exists() follows symlinks and returns False for a broken one, making it indistinguishable from a directory that was simply never fetched. A dangling apm_modules/<dep> entry is a present, corrupted install, not cold-cache state, and must still fail. _package_manifest_path now checks is_symlink() on the unresolved package_dir before calling .resolve() (which would otherwise silently substitute the symlink's target and erase the signal), and threads that boolean into _allows_missing_manifest explicitly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
d2e10e0 to
776a9af
Compare
Description
Fixes #2456: since v0.27.0 (#2390),
apm install --frozenfails on a cold cache (fresh checkout, emptyapm_modules/) for any project that combines an MCP server with a git-hostedapm_packagedependency (a package whose repo root ships its ownapm.yml), even though the lockfile is not actually stale.Root cause:
enforce_frozen(src/apm_cli/install/service.py, from #2390) callsCurrentMcpConfigView.derive(...)whenever the project has MCP state, which walks every locked dependency's on-disk manifest looking for transitively-declared MCP servers._allows_missing_manifest(src/apm_cli/integration/mcp_config_view.py) only waives an absentapm.ymlforskill_bundlepackages, or a virtual-subdirectory dependency whose materialized shape detects asclaude_skill(the #2329/#2443/#2446 cold-cache tolerance). A plain, non-virtual gitapm_packagedependency never matches either branch, so on a cold cache it hitsif not manifest_path.exists(): ... problems.append(...)unconditionally, reporting"package manifest not found at .../apm.yml; re-run 'apm install' to restore it"and failing--frozen-- even though a plainapm installon the same tree succeeds and changes nothing, andapm auditreports no drift.This is distinct from #2443 (fixed by #2446): that one is about
claude_skillpackages that never ship anapm.ymlby design (directory present, manifest absent by design). This is anapm_packagedependency (which does ship anapm.yml) whose entire install directory is simply absent because it was never fetched.Fix: generalize the cold-cache tolerance in
_allows_missing_manifest. When the package's entire install directory doesn't exist at all (not just itsapm.yml), a non-local dependency hasn't been fetched yet -- regardless ofpackage_type-- mirroring the toleranceapm audit --cialready has for the same state (#2329). A directory that is present but missing its manifest (genuine corruption) still reports a problem, and a missing local dependency directory still reports a problem (nothing fetches a local path, so it isn't cold-cache-explainable).Verified directly against the exact function
enforce_frozencalls (CurrentMcpConfigView.derive), reproducing the issue's exact error message before the fix and confirming it disappears after, while the genuine-corruption and local-dependency cases still correctly fail:Fixes #2456
Type of change
Testing
Details:
tests/unit/integration/test_mcp_config_view.py:test_cold_cache_nonvirtual_apm_package_is_tolerated-- fails onmain, passes with this fix (the [BUG] apm install --frozen does not hydrate git apm_package dependencies on a cold cache, failing with "package manifest not found" when MCP state is present #2456 regression proof).test_materialized_but_manifestless_apm_package_still_records_problem-- present-but-broken install still fails (distinguishes cold-cache tolerance from genuine corruption).test_cold_cache_local_dependency_still_records_problem-- a missing local dependency directory is not cold-cache-explainable and still fails.tests/unit/integration/test_mcp_config_view.py,tests/unit/install/test_frozen.py-- 30 passed.tests/unit/+tests/test_lockfile.py(full sweep, ~19.6k tests) -- identical pass/fail counts before and after this change (33 pre-existing failures reproduce byte-identically onmainwithout this patch: ANSI-color-escape assertion mismatches and one multi-harness-ambiguity test, all environment-specific to this machine, none touchingmcp_config_view.pyor frozen-install logic).ruff checkon changed files -- clean.Spec conformance (OpenAPM v0.1)
This PR does not add or change a normative requirement. req-lk-006 ("a frozen-install mode in which the lockfile is never written or rewritten") remains fully satisfied before and after -- this fix only corrects a false-positive in one consumer's internal heuristic for deciding whether an absent on-disk manifest is legitimate cold-cache state or genuine drift. The spec does not prescribe that heuristic; it's implementation detail, not OpenAPM-observable behaviour. The change is well under the Mode B substantive-line threshold (2 lines under
src/apm_cli/integration/), so the local detector passes without a waiver.docs/src/content/docs/specs/openapm-v0.1.mdupdated (new/changed<a id="req-XXX"></a>anchor + prose + Appendix C row).docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.ymlupdated.@pytest.mark.req("req-XXX")test undertests/spec_conformance/added or extended.CONFORMANCE.{md,json}regenerated viauv run --extra dev python -m tests.spec_conformance.gen_statementand committed.🤖 Generated with Claude Code