diff --git a/docs/src/content/docs/reference/baseline-checks.md b/docs/src/content/docs/reference/baseline-checks.md index e6fe75b59..b53728a00 100644 --- a/docs/src/content/docs/reference/baseline-checks.md +++ b/docs/src/content/docs/reference/baseline-checks.md @@ -93,9 +93,9 @@ the [policy schema](./policy-schema/). ### `config-consistency` -- **What it verifies.** That MCP server configs derived from both `dependencies.mcp` and `devDependencies.mcp` in `apm.yml` match the `mcp_configs` baseline stored in the lockfile. -- **Fails when.** A server's resolved config differs from the lockfile, a server is in the lockfile but not the manifest, or a server is in the manifest but not the lockfile. -- **Exception.** A lockfile-only server is expected when `mcp_config_provenance` identifies the sub-package that contributed it, so those transitive servers are not treated as orphans. +- **What it verifies.** That the complete current MCP declaration graph -- the root manifest, current local-package sources, and lock-bounded installed package manifests -- matches the `mcp_configs` baseline stored in the lockfile. +- **Fails when.** A server's resolved config differs from the lockfile, a source declaration was added, or a previously locked declaration was removed. +- **Ownership.** `mcp_config_provenance` identifies the package that contributed a lockfile-only server in the diagnostic; it does not exempt the removed declaration. - **Remediation.** Run `apm install` to reconcile the MCP configuration. ### `content-integrity` diff --git a/docs/src/content/docs/reference/lockfile-spec.md b/docs/src/content/docs/reference/lockfile-spec.md index 0abcbe5f1..57e50d73f 100644 --- a/docs/src/content/docs/reference/lockfile-spec.md +++ b/docs/src/content/docs/reference/lockfile-spec.md @@ -124,7 +124,7 @@ local_deployed_file_hashes: | `dependencies` | list | yes | Resolved APM packages. See [per-entry fields](#per-entry-fields). | | `mcp_servers` | list of strings | no | Names of MCP servers managed as of the last install or update, including transitively contributed servers. | | `mcp_configs` | map | no | `server_name -> resolved config dict` baseline used to detect MCP drift. | -| `mcp_config_provenance` | map | no | `server_name -> declaring package` for transitively contributed MCP servers. Used by `config-consistency` to distinguish transitive entries from orphans. | +| `mcp_config_provenance` | map | no | `server_name -> declaring package` for transitively contributed MCP servers. Used by `config-consistency` to identify the owner in drift diagnostics. | | `lsp_servers` | list of strings | no | Names of LSP servers declared in the manifest as of the last install or update. | | `lsp_configs` | map | no | `server_name -> resolved config dict` baseline used to detect LSP drift. | | `local_deployed_files` | list | no | Files this project itself contributes (sources its own primitives). Reinstall reconciles these paths with the same target rules as per-dependency `deployed_files`. See [self entry](#self-entry). | diff --git a/src/apm_cli/policy/ci_checks.py b/src/apm_cli/policy/ci_checks.py index 918b27919..6af253a32 100644 --- a/src/apm_cli/policy/ci_checks.py +++ b/src/apm_cli/policy/ci_checks.py @@ -230,44 +230,72 @@ def _check_config_consistency( manifest: APMPackage, lock: LockFile, ) -> CheckResult: - """Verify MCP server configs match lockfile baseline.""" + """Verify the complete current MCP declaration graph matches the lockfile.""" + from ..constants import APM_MODULES_DIR + from ..deps.path_anchoring import LocalResolutionError, resolve_local_dep_dir from ..drift import detect_config_drift from ..integration.mcp_integrator import MCPIntegrator + from ..models.apm_package import APMPackage - mcp_deps = manifest.get_all_mcp_dependencies() + details: list[str] = [] + mcp_deps = list(manifest.get_all_mcp_dependencies()) + for locked_dep in lock.get_package_dependencies(): + if locked_dep.source == "local": + try: + package_dir = resolve_local_dep_dir(locked_dep, lock, manifest.package_path) + except LocalResolutionError as exc: + details.append( + f"{locked_dep.repo_url}: cannot resolve local package ({exc}) -- " + "fix the resolved_by chain or re-run 'apm install'" + ) + continue + manifest_description = f"local package manifest at {package_dir / 'apm.yml'}" + else: + package_dir = locked_dep.to_dependency_ref().get_install_path( + manifest.package_path / APM_MODULES_DIR + ) + manifest_description = f"manifest for installed package {locked_dep.repo_url}" + + package_manifest = package_dir / "apm.yml" + if not package_manifest.exists(): + continue + try: + package = APMPackage.from_apm_yml(package_manifest) + except (OSError, ValueError) as exc: + details.append( + f"{locked_dep.repo_url}: cannot parse {manifest_description} ({exc}) -- " + "fix the manifest or re-run 'apm install'" + ) + continue + mcp_deps.extend(package.get_mcp_dependencies()) + + mcp_deps = MCPIntegrator.deduplicate(mcp_deps) current_configs = MCPIntegrator.get_server_configs(mcp_deps) stored_configs = lock.mcp_configs or {} # No MCP deps at all -- nothing to check - if not current_configs and not stored_configs: + if not current_configs and not stored_configs and not details: return CheckResult( name="config-consistency", passed=True, message="No MCP configs to check", ) - details: list[str] = [] - # Detect drift on servers that exist in both sets drifted = detect_config_drift(current_configs, stored_configs) for name in sorted(drifted): details.append(f"{name}: config differs from lockfile baseline") - # Servers in lockfile but not in manifest (orphaned MCP). - # Transitively-contributed servers (declared by a local-path sub-package, - # recorded with provenance in mcp_config_provenance) are exempted: the root - # manifest cannot declare them, so flagging them creates an unfixable CI - # failure. This mirrors the resolved_by-based exemption in _check_no_orphans - # (#2081; MCP-side sibling of #1846/#1855). + # Compare names symmetrically. Provenance classifies ownership for the + # diagnostic; it never exempts a removed declaration from drift detection. provenance = lock.mcp_config_provenance or {} - for name in sorted(stored_configs): - if name not in current_configs and name not in provenance: - details.append(f"{name}: in lockfile but not in manifest") - - # Servers in manifest but not in lockfile (new, not installed) - for name in sorted(current_configs): - if name not in stored_configs: - details.append(f"{name}: in manifest but not in lockfile") + for name in sorted(stored_configs.keys() - current_configs.keys()): + owner = provenance.get(name) + owner_detail = f" (declared by {owner})" if owner else "" + details.append(f"{name}: in lockfile but not in current source{owner_detail}") + + for name in sorted(current_configs.keys() - stored_configs.keys()): + details.append(f"{name}: in current source but not in lockfile") if not details: return CheckResult( diff --git a/tests/integration/test_transitive_mcp_audit_e2e.py b/tests/integration/test_transitive_mcp_audit_e2e.py index eff5afcf2..4bcac7136 100644 --- a/tests/integration/test_transitive_mcp_audit_e2e.py +++ b/tests/integration/test_transitive_mcp_audit_e2e.py @@ -88,3 +88,61 @@ def test_force_install_then_ci_audit_accepts_transitive_mcp(tmp_path: Path) -> N check for check in payload["checks"] if check["name"] == "config-consistency" ) assert config_check["passed"] is True, config_check + + +def test_ci_audit_rejects_removed_transitive_mcp_declaration(tmp_path: Path) -> None: + """Removing an installed local package's MCP declaration must fail audit.""" + project = _write_workspace(tmp_path) + + install = _run( + project, + "install", + "--force", + "--trust-transitive-mcp", + "--target", + "copilot", + "--no-policy", + ) + assert install.returncode == 0, ( + f"install stdout:\n{install.stdout}\ninstall stderr:\n{install.stderr}" + ) + + control = _run( + project, + "audit", + "--ci", + "--no-policy", + "--no-fail-fast", + "-f", + "json", + ) + assert control.returncode == 0, ( + f"audit stdout:\n{control.stdout}\naudit stderr:\n{control.stderr}" + ) + + package_manifest = project / "packages" / "agent-config" / "apm.yml" + package_manifest.write_text( + """name: agent-config +version: 1.0.0 +""", + encoding="utf-8", + ) + + audit = _run( + project, + "audit", + "--ci", + "--no-policy", + "--no-fail-fast", + "-f", + "json", + ) + assert audit.returncode == 1, f"audit stdout:\n{audit.stdout}\naudit stderr:\n{audit.stderr}" + payload = json.loads(audit.stdout) + config_check = next( + check for check in payload["checks"] if check["name"] == "config-consistency" + ) + assert config_check["passed"] is False + assert config_check["details"] == [ + "shadcn: in lockfile but not in current source (declared by agent-config)" + ] diff --git a/tests/unit/policy/test_ci_checks.py b/tests/unit/policy/test_ci_checks.py index cd073a417..4cdfadb5d 100644 --- a/tests/unit/policy/test_ci_checks.py +++ b/tests/unit/policy/test_ci_checks.py @@ -417,55 +417,186 @@ def test_fail_mcp_config_drift(self, tmp_path): assert not result.passed assert any("my-server" in d and "differs" in d for d in result.details) - def test_transitive_mcp_server_not_flagged_as_orphan(self, tmp_path): - """A server declared by a local-path sub-package is exempt from orphan. - - Topology: root -> ./packages/agent-config (local-path) which declares - a self-defined MCP server 'shadcn'. The root manifest cannot declare - the server, so it is recorded in mcp_configs with provenance pointing - at the declaring package. config-consistency must pass. Regression test - for #2081 (MCP-side sibling of #1846/#1855). - - Fix-toggle: the SAME lockfile without the provenance entry must FAIL, - proving the exemption -- not some unrelated condition -- is what flips - the result. - """ + def test_transitive_mcp_server_resolved_from_local_source(self, tmp_path): + """A local sub-package's unchanged MCP declaration matches the lock.""" from apm_cli.deps.lockfile import LockFile, get_lockfile_path from apm_cli.models.apm_package import APMPackage - # Root manifest declares only the local-path dependency, no mcp: block. + package = tmp_path / "packages" / "agent-config" + package.mkdir(parents=True) _write_apm_yml(tmp_path, deps=["./packages/agent-config"]) + (package / "apm.yml").write_text( + textwrap.dedent("""\ + name: agent-config + version: 1.0.0 + dependencies: + mcp: + - name: shadcn + registry: false + transport: stdio + command: npx + """), + encoding="utf-8", + ) - exempt_lock = textwrap.dedent("""\ - lockfile_version: '1' - generated_at: '2025-01-01T00:00:00Z' - dependencies: [] - mcp_configs: - shadcn: - name: shadcn - registry: false - transport: stdio - command: npx - mcp_config_provenance: - shadcn: '@qado/agent-config' - """) - _write_lockfile(tmp_path, exempt_lock) + _write_lockfile( + tmp_path, + textwrap.dedent("""\ + lockfile_version: '1' + generated_at: '2025-01-01T00:00:00Z' + dependencies: + - repo_url: _local/agent-config + source: local + local_path: ./packages/agent-config + deployed_files: [] + mcp_configs: + shadcn: + name: shadcn + registry: false + transport: stdio + command: npx + mcp_config_provenance: + shadcn: agent-config + """), + ) manifest = APMPackage.from_apm_yml(tmp_path / "apm.yml") lock = LockFile.read(get_lockfile_path(tmp_path)) - # Provenance must survive the YAML round-trip for the exemption to work. - assert lock.mcp_config_provenance == {"shadcn": "@qado/agent-config"} + assert lock.mcp_config_provenance == {"shadcn": "agent-config"} result = _check_config_consistency(manifest, lock) assert result.passed, result.details - # Fix-toggle: strip the provenance block; the exact same server now - # falls through the orphan branch and fails. - without_provenance = exempt_lock[: exempt_lock.index("mcp_config_provenance:")] - _write_lockfile(tmp_path, without_provenance) - lock_no_prov = LockFile.read(get_lockfile_path(tmp_path)) - assert lock_no_prov.mcp_config_provenance == {} - result_no_prov = _check_config_consistency(manifest, lock_no_prov) - assert not result_no_prov.passed - assert any("shadcn" in d and "not in manifest" in d for d in result_no_prov.details) + def test_transitive_mcp_server_resolved_from_installed_remote(self, tmp_path): + """A locked remote package's unchanged MCP declaration matches the lock.""" + package = tmp_path / "apm_modules" / "owner" / "agent-config" + package.mkdir(parents=True) + _write_apm_yml(tmp_path, deps=["owner/agent-config"]) + (package / "apm.yml").write_text( + textwrap.dedent("""\ + name: agent-config + version: 1.0.0 + dependencies: + mcp: + - name: shadcn + registry: false + transport: stdio + command: npx + """), + encoding="utf-8", + ) + _write_lockfile( + tmp_path, + textwrap.dedent("""\ + lockfile_version: '1' + generated_at: '2025-01-01T00:00:00Z' + dependencies: + - repo_url: owner/agent-config + package_type: apm_package + deployed_files: [] + mcp_configs: + shadcn: + name: shadcn + registry: false + transport: stdio + command: npx + mcp_config_provenance: + shadcn: agent-config + """), + ) + from apm_cli.deps.lockfile import LockFile, get_lockfile_path + from apm_cli.models.apm_package import APMPackage + + manifest = APMPackage.from_apm_yml(tmp_path / "apm.yml") + lock = LockFile.read(get_lockfile_path(tmp_path)) + result = _check_config_consistency(manifest, lock) + + assert result.passed, result.details + + @pytest.mark.parametrize( + ("dependency", "locked_dependency", "package_parts", "expected_detail"), + [ + ( + "./packages/broken", + """\ + - repo_url: _local/broken + source: local + local_path: ./packages/broken + deployed_files: [] + """, + ("packages", "broken"), + "cannot parse local package manifest at", + ), + ( + "owner/broken", + """\ + - repo_url: owner/broken + package_type: apm_package + deployed_files: [] + """, + ("apm_modules", "owner", "broken"), + "cannot parse manifest for installed package owner/broken", + ), + ], + ) + def test_manifest_parse_error_identifies_package_source( + self, + tmp_path, + dependency, + locked_dependency, + package_parts, + expected_detail, + ): + """Manifest parse failures identify local and installed package sources.""" + package = tmp_path.joinpath(*package_parts) + package.mkdir(parents=True) + (package / "apm.yml").write_text("name: [invalid\n", encoding="utf-8") + _write_apm_yml(tmp_path, deps=[dependency]) + lock_dependency = textwrap.indent(textwrap.dedent(locked_dependency).strip(), " ") + _write_lockfile( + tmp_path, + "lockfile_version: '1'\n" + "generated_at: '2025-01-01T00:00:00Z'\n" + f"dependencies:\n{lock_dependency}\n", + ) + from apm_cli.deps.lockfile import LockFile, get_lockfile_path + from apm_cli.models.apm_package import APMPackage + + manifest = APMPackage.from_apm_yml(tmp_path / "apm.yml") + lock = LockFile.read(get_lockfile_path(tmp_path)) + result = _check_config_consistency(manifest, lock) + + assert not result.passed + assert len(result.details) == 1 + assert expected_detail in result.details[0] + if dependency.startswith("."): + assert str(package / "apm.yml") in result.details[0] + + def test_manifestless_local_skill_does_not_fail_mcp_consistency(self, tmp_path): + """A locked manifestless skill cannot contribute MCP declarations.""" + package = tmp_path / "skills" / "example" + package.mkdir(parents=True) + (package / "SKILL.md").write_text("# Example\n", encoding="utf-8") + _write_apm_yml(tmp_path, deps=["./skills/example"]) + _write_lockfile( + tmp_path, + textwrap.dedent("""\ + lockfile_version: '1' + generated_at: '2025-01-01T00:00:00Z' + dependencies: + - repo_url: _local/example + source: local + local_path: ./skills/example + package_type: skill_bundle + deployed_files: [] + """), + ) + from apm_cli.deps.lockfile import LockFile, get_lockfile_path + from apm_cli.models.apm_package import APMPackage + + manifest = APMPackage.from_apm_yml(tmp_path / "apm.yml") + lock = LockFile.read(get_lockfile_path(tmp_path)) + result = _check_config_consistency(manifest, lock) + + assert result.passed, result.details def test_genuine_orphan_mcp_still_flagged(self, tmp_path): """Provenance must not mask a real orphan (server absent from manifest @@ -495,7 +626,7 @@ def test_genuine_orphan_mcp_still_flagged(self, tmp_path): lock = LockFile.read(get_lockfile_path(tmp_path)) result = _check_config_consistency(manifest, lock) assert not result.passed - assert any("stale" in d and "not in manifest" in d for d in result.details) + assert any("stale" in d and "not in current source" in d for d in result.details) # -- Content integrity ----------------------------------------------