diff --git a/src/apm_cli/primitives/discovery.py b/src/apm_cli/primitives/discovery.py index 707b1f17d..d307b4abe 100644 --- a/src/apm_cli/primitives/discovery.py +++ b/src/apm_cli/primitives/discovery.py @@ -287,13 +287,51 @@ def get_dependency_declaration_order(base_dir: str) -> list[str]: # This matches our org-namespaced directory structure dependency_names.append(dep.repo_url) - # Include transitive dependencies from apm.lock - # Direct deps from apm.yml have priority; transitive deps are appended - lockfile_paths = LockFile.installed_paths_for_project(Path(base_dir)) + # Include transitive dependencies + local-bundle slugs from the + # lockfile. Read it once and reuse the parsed object for both + # the transitive-paths walk and the ``local_deployed_files`` + # slug derivation (issue #1363) to avoid duplicate YAML parses + # on every compile. + project_root = Path(base_dir) + lockfile_path = project_root / "apm.lock.yaml" + if not lockfile_path.exists(): + legacy = project_root / "apm.lock" + if legacy.exists(): + lockfile_path = legacy + lock = LockFile.read(lockfile_path) if lockfile_path.exists() else None + direct_set = set(dependency_names) - for path in lockfile_paths: - if path not in direct_set: - dependency_names.append(path) + if lock is not None: + for path in lock.get_installed_paths(project_root / "apm_modules"): + if path not in direct_set: + dependency_names.append(path) + + # Local-bundle install stages instructions under + # ``apm_modules//.apm/...`` but intentionally does NOT + # mutate ``apm.yml`` (services.py:489-490), so the scan loop + # would otherwise never visit those staged dirs and + # ``apm compile`` would produce no output for compile-only + # targets (opencode, codex, gemini). + # + # Provenance is anchored to the lockfile record -- a stray + # directory under ``apm_modules/`` without a lockfile entry must + # not be discovered (defends against phantom-content injection + # and stale-debris drift). + if lock is not None: + local_slugs: set[str] = set() + for deployed in lock.local_deployed_files: + # Match ``apm_modules//.apm/...`` only. Other + # deployed files (``.github/instructions/...``, + # ``.agents/skills/...``) are not bundle staging + # markers and must not produce phantom slugs. + parts = Path(deployed).parts + if len(parts) >= 3 and parts[0] == "apm_modules" and parts[2] == ".apm": + local_slugs.add(parts[1]) + seen = set(dependency_names) + for slug in sorted(local_slugs): + if slug not in seen: + dependency_names.append(slug) + seen.add(slug) return dependency_names diff --git a/tests/integration/test_install_local_bundle_e2e.py b/tests/integration/test_install_local_bundle_e2e.py index cc16c7b0a..c186d5021 100644 --- a/tests/integration/test_install_local_bundle_e2e.py +++ b/tests/integration/test_install_local_bundle_e2e.py @@ -18,6 +18,7 @@ import subprocess import tarfile from pathlib import Path +from typing import ClassVar from unittest.mock import patch import pytest @@ -1084,3 +1085,124 @@ def test_bundle_mcp_dry_run_does_not_call_integrator( assert result.exit_code == 0, result.output mock_install.assert_not_called() + + +# --------------------------------------------------------------------------- +# Regression for issue #1363 -- local-bundle compile round-trip +# --------------------------------------------------------------------------- + + +class TestLocalBundleCompileRoundTrip: + """Regression coverage for issue #1363. + + Before the fix, ``apm install -t `` + staged instructions under ``apm_modules//.apm/instructions/`` but + ``apm compile`` never discovered them, because the discovery scan only + walked paths declared in ``apm.yml`` deps + ``apm.lock.yaml`` + ``dependencies[]``. Local-bundle install intentionally does NOT mutate + ``apm.yml`` (services.py:489-490), so the staged content was invisible + and compile produced no ``AGENTS.md`` / ``GEMINI.md``. + + The fix derives bundle slugs from the lockfile's top-level + ``local_deployed_files`` field (already written by + ``local_bundle_handler.py:194-199``) and surfaces them to the discovery + scan. This test exercises the full install -> compile pipeline for + every compile-only target the bug affects. + """ + + OUTPUT_FILE_BY_TARGET: ClassVar[dict[str, str]] = { + "opencode": "AGENTS.md", + "codex": "AGENTS.md", + "gemini": "GEMINI.md", + } + + @pytest.mark.parametrize("target", ["opencode", "codex", "gemini"]) + def test_install_then_compile_produces_output_with_staged_instruction( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + target: str, + ) -> None: + """For every compile-only target, install a local bundle whose + only payload is an instruction, then run ``apm compile`` and + assert the target's output file is produced with the staged + instruction content embedded. + """ + from apm_cli.integration.targets import KNOWN_TARGETS + + # Bundle with a single distinctive instruction so we can assert + # the body is present in the compiled output. + marker = "PEP8-MARKER-issue-1363" + files = { + "instructions/style.instructions.md": ( + "---\n" + "description: Style guide for the bundle\n" + "applyTo: '**/*.py'\n" + "---\n\n" + f"# Style Guide\nFollow PEP8. {marker}\n" + ), + } + bundle = _make_plugin_bundle( + tmp_path / "src", + plugin_id="bundle-1363", + pack_target="all", + files=files, + ) + project = _make_project(tmp_path / "dst") + # Pre-create the target's root_dir so auto-detect resolves to it + # (mirrors a real project configured for that IDE). + (project / KNOWN_TARGETS[target].root_dir).mkdir(parents=True, exist_ok=True) + + install_result = _invoke_install(project, str(bundle), monkeypatch=monkeypatch) + assert install_result.exit_code == 0, ( + f"install failed for target={target}: {install_result.output!r}" + ) + # Sanity: the staging path the fix relies on must exist on disk. + staged = ( + project + / "apm_modules" + / "bundle-1363" + / ".apm" + / "instructions" + / "style.instructions.md" + ) + assert staged.is_file(), ( + f"local-bundle install did not stage at {staged} for target={target}" + ) + + # Now run `apm compile --target ` in the project dir. + # Pin cwd explicitly rather than rely on a side-effect from + # ``_invoke_install`` (which monkeypatch.chdir'd into the + # project): future refactors of the install helper must not + # silently break this E2E. + monkeypatch.chdir(project) + runner = CliRunner() + compile_result = runner.invoke( + cli, + ["compile", "--target", target], + catch_exceptions=False, + ) + assert compile_result.exit_code == 0, ( + f"compile failed for target={target}: {compile_result.output!r}" + ) + + # The target's compile output file must exist AND contain the + # marker from the staged instruction. This is the regression + # signal: before the fix, the output file was not produced at + # all ("Compilation completed but produced no output files"). + out_name = self.OUTPUT_FILE_BY_TARGET[target] + out_path = project / out_name + assert out_path.is_file(), ( + f"compile did not produce {out_name} for target={target}; " + f"output: {compile_result.output!r}" + ) + body = out_path.read_text(encoding="utf-8") + # For gemini, GEMINI.md is a thin pointer to AGENTS.md + # (``@./AGENTS.md``), so the staged content lands in AGENTS.md + # itself. Walk that pointer when present. + if target == "gemini" and "@./AGENTS.md" in body: + body = (project / "AGENTS.md").read_text(encoding="utf-8") + assert marker in body, ( + f"compile did not include staged instruction marker in {out_name} " + f"for target={target}; body head: {body[:400]!r}" + ) diff --git a/tests/unit/primitives/test_discovery_parser.py b/tests/unit/primitives/test_discovery_parser.py index af984f1cb..74d2223e3 100644 --- a/tests/unit/primitives/test_discovery_parser.py +++ b/tests/unit/primitives/test_discovery_parser.py @@ -593,26 +593,196 @@ def test_transitive_deps_appended_deduped(self): """Transitive deps from lockfile are appended but not duplicated.""" apm_yml = Path(self.tmp) / "apm.yml" apm_yml.write_text("name: test\n") + # Write a real lockfile so discovery's single LockFile.read() call + # picks up the transitive entry. apm_modules dir doesn't need to + # actually exist for get_installed_paths to return the entries. + (Path(self.tmp) / "apm.lock.yaml").write_text( + "version: 1\n" + "dependencies:\n" + " - repo_url: owner/direct-dep\n" + " resolved_ref: HEAD\n" + " resolved_commit: 0\n" + " version: 0.0.0\n" + " depth: 0\n" + " source: registry\n" + " - repo_url: owner/transitive-dep\n" + " resolved_ref: HEAD\n" + " resolved_commit: 0\n" + " version: 0.0.0\n" + " depth: 1\n" + " source: registry\n" + ) mock_dep = MagicMock() mock_dep.alias = None mock_dep.is_virtual = False mock_dep.repo_url = "owner/direct-dep" mock_package = MagicMock() mock_package.get_apm_dependencies.return_value = [mock_dep] - with ( - patch( - "apm_cli.primitives.discovery.APMPackage.from_apm_yml", - return_value=mock_package, - ), - patch( - "apm_cli.primitives.discovery.LockFile.installed_paths_for_project", - return_value=["owner/direct-dep", "owner/transitive-dep"], - ), + with patch( + "apm_cli.primitives.discovery.APMPackage.from_apm_yml", + return_value=mock_package, ): result = get_dependency_declaration_order(self.tmp) self.assertEqual(result, ["owner/direct-dep", "owner/transitive-dep"]) +class TestLocalBundleStagedSlugs(unittest.TestCase): + """Regression tests for issue #1363. + + Local-bundle install stages instructions under + ``apm_modules//.apm/instructions/`` and records the deployed paths + in the lockfile's top-level ``local_deployed_files`` field. The install + intentionally does NOT mutate ``apm.yml`` (services.py:489-490), so + ``scan_dependency_primitives`` previously could not see the staged + primitives, and ``apm compile`` produced no output for compile-only + targets (opencode, codex, gemini). + + The fix derives bundle slugs from ``local_deployed_files`` entries that + match ``apm_modules//.apm/...`` and feeds them into the discovery + scan list, so compile picks them up while the apm.yml contract stays + untouched. Provenance is anchored to the lockfile record, not to a blind + walk of ``apm_modules/`` -- a stray directory under ``apm_modules/`` + without a lockfile record must NOT be discovered. + """ + + def setUp(self): + self.tmp = tempfile.mkdtemp() + + def tearDown(self): + import shutil + + shutil.rmtree(self.tmp, ignore_errors=True) + + def _write_local_bundle_lockfile(self, deployed_files: list[str]) -> None: + """Write an apm.lock.yaml with ``local_deployed_files`` populated. + + Mirrors what ``install_local_bundle`` writes in + ``local_bundle_handler.py:194-199``. + """ + import yaml + + data: dict = { + "lockfile_version": "1", + "generated_at": "2025-01-01T00:00:00+00:00", + } + if deployed_files: + data["local_deployed_files"] = sorted(deployed_files) + (Path(self.tmp) / "apm.lock.yaml").write_text( + yaml.dump(data, default_flow_style=False), encoding="utf-8" + ) + + def test_staged_bundle_slug_appended_when_apm_yml_empty(self): + """A local bundle staged under apm_modules//.apm/ appears in + the declaration order even with no apm.yml dependency entry. + """ + apm_yml = Path(self.tmp) / "apm.yml" + apm_yml.write_text("name: test\nversion: 1.0.0\n") + self._write_local_bundle_lockfile( + [ + "apm_modules/my-bundle/.apm/instructions/style.md", + "apm_modules/my-bundle/.apm/instructions/style.instructions.md", + ] + ) + result = get_dependency_declaration_order(self.tmp) + self.assertIn("my-bundle", result) + + def test_no_lockfile_record_no_slug(self): + """A stray apm_modules//.apm/ directory with NO lockfile + record must NOT be surfaced. This defends the security invariant + that discovery is gated by lockfile provenance, not by raw + on-disk presence (otherwise a hostile actor or stale debris + could inject content into compile output). + """ + apm_yml = Path(self.tmp) / "apm.yml" + apm_yml.write_text("name: test\nversion: 1.0.0\n") + # No lockfile written at all. + # Even if we physically stage files on disk, discovery must not + # surface them without a lockfile record. + staged = Path(self.tmp) / "apm_modules" / "phantom" / ".apm" / "instructions" + staged.mkdir(parents=True) + (staged / "evil.instructions.md").write_text(INSTRUCTION_CONTENT) + result = get_dependency_declaration_order(self.tmp) + self.assertNotIn("phantom", result) + + def test_declared_dep_wins_over_local_bundle_with_same_slug(self): + """When both a declared dep and a local-bundle slug share a name, + the declared dep appears first (priority). The local-bundle slug + is deduplicated against the existing entry. + """ + apm_yml = Path(self.tmp) / "apm.yml" + apm_yml.write_text("name: test\n") + mock_dep = MagicMock() + mock_dep.alias = "shared-name" + mock_dep.is_virtual = False + mock_package = MagicMock() + mock_package.get_apm_dependencies.return_value = [mock_dep] + self._write_local_bundle_lockfile(["apm_modules/shared-name/.apm/instructions/x.md"]) + with patch( + "apm_cli.primitives.discovery.APMPackage.from_apm_yml", + return_value=mock_package, + ): + result = get_dependency_declaration_order(self.tmp) + # "shared-name" appears exactly once, and the alias (declared) + # comes first. + self.assertEqual(result.count("shared-name"), 1) + self.assertEqual(result[0], "shared-name") + + def test_multiple_local_bundles_sorted_deterministically(self): + """Two local bundles surface in deterministic order (alphabetical + by slug) so compile output is reproducible across runs. + """ + apm_yml = Path(self.tmp) / "apm.yml" + apm_yml.write_text("name: test\nversion: 1.0.0\n") + self._write_local_bundle_lockfile( + [ + "apm_modules/bundle-b/.apm/instructions/b.md", + "apm_modules/bundle-a/.apm/instructions/a.md", + ] + ) + result = get_dependency_declaration_order(self.tmp) + # bundle-a appears before bundle-b regardless of file insertion + # order in the lockfile list. + idx_a = result.index("bundle-a") + idx_b = result.index("bundle-b") + self.assertLess(idx_a, idx_b) + + def test_non_apm_modules_paths_ignored(self): + """``local_deployed_files`` also records files outside + apm_modules/ (e.g. .github/instructions/x.md from the same bundle + install). Those must NOT produce phantom slugs. + """ + apm_yml = Path(self.tmp) / "apm.yml" + apm_yml.write_text("name: test\nversion: 1.0.0\n") + self._write_local_bundle_lockfile( + [ + ".github/instructions/style.md", + ".agents/skills/coding/SKILL.md", + "apm_modules/real-bundle/.apm/instructions/x.md", + ] + ) + result = get_dependency_declaration_order(self.tmp) + # The legitimate apm_modules entry must produce its slug. + self.assertIn("real-bundle", result) + # The non-apm_modules entries must NOT produce phantom slugs -- + # check absence of plausible spoofs derived from their paths + # (filenames or first-segment dir names) without forbidding + # unrelated dependency entries from coexisting. + for phantom in ("style", ".github", "coding", ".agents"): + self.assertNotIn(phantom, result) + + def test_non_instructions_apm_subdirs_also_recognized(self): + """Future-proof: bundles may stage other primitive types under + apm_modules//.apm/ (e.g. context/, agents/). The slug + derivation must trigger on any apm_modules//.apm/* path, + not only instructions/. + """ + apm_yml = Path(self.tmp) / "apm.yml" + apm_yml.write_text("name: test\nversion: 1.0.0\n") + self._write_local_bundle_lockfile(["apm_modules/ctx-bundle/.apm/context/notes.context.md"]) + result = get_dependency_declaration_order(self.tmp) + self.assertIn("ctx-bundle", result) + + class TestScanLocalPrimitives(unittest.TestCase): """Tests for scan_local_primitives."""