diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ef4721095..1f5efe58ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **BREAKING:** after consumers re-vendor the shared gh-aw `apm.md`, its import requires an explicit `target` instead of deprecated `all`; `apm-action` otherwise writes `all` into the isolated `apm.yml`, where it degrades to auto-detection without harness markers. Set the workflow engine's target and recompile; see the [gh-aw migration recipe](https://microsoft.github.io/apm/integrations/gh-aw/#shared-apmmd-import-recommended). (#2706) - Re-vendored shared gh-aw workflows now default to APM 0.28.0 for both pack and restore, the version used for the recorded `microsoft/apm-action@v1.10.0` compatibility proof, not the latest CLI release; an explicit `apm-version` still overrides it. (#2706) +### Fixed + +- `apm install` now explains skipped symlinked agent sources and points to the real-files remedy, without changing which files deploy. (#2932) + ### Security - The shared gh-aw APM pack job now declares `contents: read` (previously `permissions: {}`), the minimum the explicit built-in-token path needs. No write scope is added, and the token is not forwarded to restore or agent jobs. (#2706) diff --git a/CONFORMANCE.json b/CONFORMANCE.json index b2fbc74e4d..52d187e491 100644 --- a/CONFORMANCE.json +++ b/CONFORMANCE.json @@ -1254,8 +1254,10 @@ "oracle": "tests/fixtures/spec-conformance/source-plan/req-sc-015.json", "section": "10.16", "status": "active", - "test_count": 3, + "test_count": 5, "tests": [ + "tests/spec_conformance/test_manifest_reqs.py::test_agent_skip_diagnostic_preserves_authorized_scan_and_materialization[False]", + "tests/spec_conformance/test_manifest_reqs.py::test_agent_skip_diagnostic_preserves_authorized_scan_and_materialization[True]", "tests/spec_conformance/test_manifest_reqs.py::test_authorized_source_plan_fixture_oracle_covers_symlinked_content", "tests/spec_conformance/test_manifest_reqs.py::test_authorized_source_plan_limits_scanning_and_skill_materialization", "tests/spec_conformance/test_manifest_reqs.py::test_authorized_source_plan_requirement_covers_reintegration_and_symlinks" diff --git a/CONFORMANCE.md b/CONFORMANCE.md index a6c0ce79fd..4fdd347933 100644 --- a/CONFORMANCE.md +++ b/CONFORMANCE.md @@ -144,7 +144,7 @@ Repository-coordinate segments are case-insensitive for `github.com`, GitHub Ent | [req-sc-012](docs/src/content/docs/specs/openapm-v0.1.md#req-sc-012) | MUST | 10.14 | consumer | active | 1 | - | | [req-sc-013](docs/src/content/docs/specs/openapm-v0.1.md#req-sc-013) | MUST | 10.3 | consumer | active | 1 | - | | [req-sc-014](docs/src/content/docs/specs/openapm-v0.1.md#req-sc-014) | MUST | 10.15 | consumer | active | 1 | - | -| [req-sc-015](docs/src/content/docs/specs/openapm-v0.1.md#req-sc-015) | MUST | 10.16 | consumer | active | 3 | tests/fixtures/spec-conformance/source-plan/req-sc-015.json | +| [req-sc-015](docs/src/content/docs/specs/openapm-v0.1.md#req-sc-015) | MUST | 10.16 | consumer | active | 5 | tests/fixtures/spec-conformance/source-plan/req-sc-015.json | | [req-tg-001](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-001) | MUST | 8.4 | consumer | active | 1 | - | | [req-tg-002](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-002) | MUST | 8.5 | consumer | active | 1 | - | | [req-tg-003](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-003) | MUST | 8.5 | consumer | active | 1 | - | diff --git a/docs/src/content/docs/consumer/install-packages.md b/docs/src/content/docs/consumer/install-packages.md index 5a591f4cb5..d3e680406f 100644 --- a/docs/src/content/docs/consumer/install-packages.md +++ b/docs/src/content/docs/consumer/install-packages.md @@ -222,6 +222,13 @@ For the full flag reference, run `apm install --help` or see - **Drift between `apm_modules/` and the lockfile.** Run `apm audit --ci` locally to reproduce the CI gate; see [Update and refresh](../update-and-refresh/) to recover. +- **Skipped symlinked agent source.** `apm install` warns and skips agent + files or directories that are symlinks (for example + `.apm/agents -> ../agents`). Fix by shipping real files and directories in + the source package, then rerun `apm install`. Do not edit `apm_modules/` -- + cached edits are not a durable fix. For a third-party package, + ask the author to publish real sources. See + [Instructions and agents](../../producer/author-primitives/instructions-and-agents/#agents). Once your dependencies are installed, scripts run them. [Run scripts](../run-scripts/) shows how to wire `apm.yml`'s diff --git a/docs/src/content/docs/producer/author-primitives/instructions-and-agents.md b/docs/src/content/docs/producer/author-primitives/instructions-and-agents.md index 0f442e8c38..fb17686aca 100644 --- a/docs/src/content/docs/producer/author-primitives/instructions-and-agents.md +++ b/docs/src/content/docs/producer/author-primitives/instructions-and-agents.md @@ -139,6 +139,14 @@ my-package/ File names end in `.agent.md` and live under `.apm/agents/`. +For own-project and Git-backed package installs, symlinked agent source +files and directories (including `.apm/agents -> ../agents`) are skipped. +`apm install` warns with the skipped source path. Use real files and +directories under `.apm/agents/` or real `*.agent.md` files at the package +root, then rerun `apm install`. Local-path dependencies (`./...` or `../...`) +still work: contained symlinks are validated and copied as real files into +`apm_modules/` before agent discovery. + ### Frontmatter ```markdown diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index 3a4fa46d4f..894b24a6d0 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -98,6 +98,14 @@ Per-primitive scan paths for `apm install`: | agent | `.apm/agents/` | Yes: `*.agent.md` at root | | skill | `.apm/skills//` | Yes: `skills//` (SKILL_BUNDLE or MARKETPLACE_PLUGIN) | +Own-project and Git-backed package installs skip symlinked agent source +files and directories, including `.apm/agents -> ../agents`. +`apm install` warns with the skipped source path. Use real files and +directories under `.apm/agents/` or real `*.agent.md` files at root, then +rerun `apm install`. Local-path dependencies (`./...` or `../...`) still work: +contained symlinks are validated and copied as real files into +`apm_modules/` before agent discovery. + **Recommendation for marketplace publishers:** use `.apm//` for every primitive. This is the only layout that is symmetric between `apm pack` and `apm install`. diff --git a/src/apm_cli/install/deployable_source_plan.py b/src/apm_cli/install/deployable_source_plan.py index 1f2b04340e..2294f3a10a 100644 --- a/src/apm_cli/install/deployable_source_plan.py +++ b/src/apm_cli/install/deployable_source_plan.py @@ -9,13 +9,14 @@ from __future__ import annotations import os -from collections.abc import Iterator +from collections.abc import Callable, Iterator from dataclasses import dataclass from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any from apm_cli.install.cache_pin import MARKER_FILENAME from apm_cli.models.dependency.subsets import skill_subset_filter_tokens +from apm_cli.utils.diagnostics import printable_ascii_text from apm_cli.utils.path_security import ( PathTraversalError, ensure_path_within_resolved, @@ -23,14 +24,23 @@ ) from apm_cli.utils.paths import portable_relpath +if TYPE_CHECKING: + from apm_cli.utils.diagnostics import DiagnosticCollector -def _is_safe_source_path(path: Path, source_root: Path) -> bool: - """Return whether a source candidate stays in the real package tree.""" + +def _is_safe_source_path( + path: Path, + source_root: Path, + on_symlink: Callable[[Path], None] | None = None, +) -> bool: + """Check containment; notify on_symlink only for symlink-component rejection.""" try: path.relative_to(source_root) except ValueError: return False if has_symlink_component(source_root, path): + if on_symlink is not None: + on_symlink(path) return False try: ensure_path_within_resolved(path, source_root) @@ -73,6 +83,8 @@ def create( canvas_approved: bool, skip_bin: bool, plugin_bin_deployable: bool = False, + diagnostics: DiagnosticCollector | None = None, + package_name: str = "", ) -> DeployableSourcePlan: """Build the authorized deploy set after all deployment gates resolve.""" source_root = Path(package_info.install_path).resolve() @@ -81,19 +93,36 @@ def create( hook_source_selection = None target_primitives = {primitive for target in targets for primitive in target.primitives} - def add_file(path: Path) -> None: - if _is_safe_source_path(path, source_root) and path.is_file(): + def warn_agent_symlink(path: Path) -> None: + if diagnostics is not None: + # Name the rejected link, not the target portable_relpath resolves. + relative = path.relative_to(source_root).as_posix() + diagnostics.warn( + message=( + "Skipped symlinked agent source: " + f"{printable_ascii_text(relative)}. " + "Symlinked agent sources are not deployed. " + "Use real files and directories in .apm/agents/ " + "(or real *.agent.md files at the package root), then rerun apm install." + ), + package=printable_ascii_text(package_name), + ) + + def add_file(path: Path, on_symlink: Callable[[Path], None] | None = None) -> None: + if _is_safe_source_path(path, source_root, on_symlink) and path.is_file(): paths.add(portable_relpath(path, source_root)) - def tree_files(root: Path) -> Iterator[Path]: - if not _is_safe_source_path(root, source_root) or not root.is_dir(): + def tree_files( + root: Path, on_symlink: Callable[[Path], None] | None = None + ) -> Iterator[Path]: + if not _is_safe_source_path(root, source_root, on_symlink) or not root.is_dir(): return for parent, directory_names, file_names in os.walk(root, followlinks=False): parent_path = Path(parent) directory_names[:] = [ name for name in directory_names - if _is_safe_source_path(parent_path / name, source_root) + if _is_safe_source_path(parent_path / name, source_root, on_symlink) ] yield from (parent_path / name for name in file_names) @@ -101,25 +130,29 @@ def add_tree(root: Path) -> None: for path in tree_files(root): add_file(path) - def add_matching_files(root: Path, pattern: str) -> None: - for path in tree_files(root): + def add_matching_files( + root: Path, pattern: str, on_symlink: Callable[[Path], None] | None = None + ) -> None: + for path in tree_files(root, on_symlink): if path.match(pattern): - add_file(path) + add_file(path, on_symlink) - def add_direct_matching_files(root: Path, pattern: str) -> None: - if not _is_safe_source_path(root, source_root) or not root.is_dir(): + def add_direct_matching_files( + root: Path, pattern: str, on_symlink: Callable[[Path], None] | None = None + ) -> None: + if not _is_safe_source_path(root, source_root, on_symlink) or not root.is_dir(): return for path in root.iterdir(): if path.match(pattern): - add_file(path) + add_file(path, on_symlink) if "prompts" in target_primitives or "commands" in target_primitives: add_direct_matching_files(source_root, "*.prompt.md") add_matching_files(source_root / ".apm" / "prompts", "*.prompt.md") if "agents" in target_primitives: - add_direct_matching_files(source_root, "*.agent.md") - add_matching_files(source_root / ".apm" / "agents", "*.md") + add_direct_matching_files(source_root, "*.agent.md", warn_agent_symlink) + add_matching_files(source_root / ".apm" / "agents", "*.md", warn_agent_symlink) if "instructions" in target_primitives: add_matching_files(source_root / ".apm" / "instructions", "*.instructions.md") diff --git a/src/apm_cli/install/services.py b/src/apm_cli/install/services.py index 5cf7a726c1..606003241e 100644 --- a/src/apm_cli/install/services.py +++ b/src/apm_cli/install/services.py @@ -350,6 +350,8 @@ def _reconcile_excluded_targets() -> None: hooks_approved=_hooks_approved, canvas_approved=_canvas_approved or is_first_party, skip_bin=_skip_bin, + diagnostics=diagnostics, + package_name=package_name, plugin_bin_deployable=_plugin_bin_deployable( package_info, targets, diff --git a/tests/spec_conformance/test_manifest_reqs.py b/tests/spec_conformance/test_manifest_reqs.py index a4c12ad879..2c21109a28 100644 --- a/tests/spec_conformance/test_manifest_reqs.py +++ b/tests/spec_conformance/test_manifest_reqs.py @@ -1296,6 +1296,66 @@ def materialize(destination: Path, source_plan: DeployableSourcePlan) -> frozens ) +@pytest.mark.req("req-sc-015") +@pytest.mark.parametrize("directory_link", [False, True]) +def test_agent_skip_diagnostic_preserves_authorized_scan_and_materialization( + tmp_path: Path, directory_link: bool +) -> None: + """req-sc-015 owns exclusion; the paired warning is APM UX, not a spec mandate.""" + from apm_cli.install.deployable_source_plan import DeployableSourcePlan + from apm_cli.models.apm_package import PackageInfo, PackageType + from apm_cli.security.gate import BLOCK_POLICY + + source = tmp_path / "source" + (source / ".apm").mkdir(parents=True) + (source / "safe.agent.md").write_text( + "---\nname: safe\ndescription: Safe agent.\n---\nBody.\n", encoding="utf-8" + ) + hidden = source / "unselected" + hidden.mkdir() + (hidden / "hidden.agent.md").write_text("not scanned\u202e\n", encoding="utf-8") + link = source / (".apm/agents" if directory_link else "linked.agent.md") + link.symlink_to( + hidden if directory_link else hidden / "hidden.agent.md", + target_is_directory=directory_link, + ) + package = PackageInfo( + package=APMPackage(name="source", version="1.0.0"), + install_path=source, + package_type=PackageType.APM_PACKAGE, + ) + diagnostics = DiagnosticCollector() + target = KNOWN_TARGETS["claude"] + plan = DeployableSourcePlan.create( + package, + [target], + skill_subset=None, + hooks_approved=False, + canvas_approved=False, + skip_bin=True, + diagnostics=diagnostics, + package_name="source", + ) + scan = plan.scan_security(policy=BLOCK_POLICY) + assert plan.paths == scan.scanned_files == frozenset({"safe.agent.md"}) + assert not scan.has_findings + + destination = tmp_path / "destination" + (destination / ".claude").mkdir(parents=True) + result = AgentIntegrator().integrate_agents_for_target( + target, package, destination, source_plan=plan, diagnostics=diagnostics + ) + expected = destination / ".claude" / "agents" / "safe.md" + assert result.files_integrated == 1 + assert result.target_paths == [expected] + assert list((destination / ".claude" / "agents").iterdir()) == [expected] + warnings = diagnostics.by_category()[CATEGORY_WARNING] + assert len(warnings) == 1 + assert warnings[0].message.startswith( + f"Skipped symlinked agent source: {link.relative_to(source).as_posix()}." + ) + + @pytest.mark.req("req-sc-015") def test_authorized_source_plan_requirement_covers_reintegration_and_symlinks() -> None: """The citation names every lifecycle and excludes symlink source entries.""" diff --git a/tests/unit/install/test_agent_symlink_diagnostics.py b/tests/unit/install/test_agent_symlink_diagnostics.py new file mode 100644 index 0000000000..5d136b2633 --- /dev/null +++ b/tests/unit/install/test_agent_symlink_diagnostics.py @@ -0,0 +1,242 @@ +"""Real-filesystem regressions for diagnostic-only agent symlink rejection.""" + +from pathlib import Path +from unittest.mock import patch + +import pytest +from click.testing import CliRunner + +from apm_cli.cli import cli +from apm_cli.install.deployable_source_plan import DeployableSourcePlan +from apm_cli.install.services import ( + IntegratorBundle, + integrate_local_content, + integrate_package_primitives, +) +from apm_cli.integration.agent_integrator import AgentIntegrator +from apm_cli.integration.command_integrator import CommandIntegrator +from apm_cli.integration.hook_integrator import HookIntegrator +from apm_cli.integration.instruction_integrator import InstructionIntegrator +from apm_cli.integration.prompt_integrator import PromptIntegrator +from apm_cli.integration.skill_integrator import SkillIntegrator +from apm_cli.integration.targets import KNOWN_TARGETS +from apm_cli.models.apm_package import APMPackage, PackageInfo, PackageType +from apm_cli.utils.diagnostics import DiagnosticCollector + +pytestmark = pytest.mark.component + + +@pytest.mark.parametrize("verbose", [False, True]) +def test_own_project_cli_reports_symlink_skip_on_install_and_reinstall( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, verbose: bool +) -> None: + """The default and verbose CLI expose the cause without ledger expansion.""" + monkeypatch.chdir(tmp_path) + (tmp_path / "apm.yml").write_text( + "name: probe\nversion: 0.0.0\ndependencies:\n apm: []\n mcp: []\n", + encoding="utf-8", + ) + (tmp_path / ".claude").mkdir() + (tmp_path / ".apm").mkdir() + (tmp_path / "agents").mkdir() + (tmp_path / "agents" / "repro.agent.md").write_text( + "---\nname: repro\ndescription: A repro agent.\n---\n\nBody.\n", + encoding="utf-8", + ) + (tmp_path / ".apm" / "agents").symlink_to("../agents", target_is_directory=True) + args = ["install", "--target", "claude", *(["--verbose"] if verbose else [])] + runner = CliRunner() + for _ in range(2): + result = runner.invoke(cli, args) + assert result.exit_code == 0, result.output + assert result.output.count("Skipped symlinked agent source: .apm/agents") == 1 + assert "real files" in result.output + assert not (tmp_path / ".claude" / "agents").exists() + assert not (tmp_path / "apm.lock.yaml").exists() + + +@pytest.mark.parametrize("own_project", [True, False], ids=["own-project", "git-package"]) +@pytest.mark.parametrize("linked", [True, False], ids=["symlink", "real-directory"]) +def test_agent_source_skip_is_actionable_without_deployment_expansion( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + own_project: bool, + linked: bool, +) -> None: + """Both install routes warn once, retain empty ledgers, and deploy real sources.""" + project = tmp_path / "project" + (project / ".claude").mkdir(parents=True) + (project / ".github").mkdir() + source = project if own_project else project / "apm_modules" / "owner" / "bundle" + agents = source / ("agents" if linked else ".apm/agents") + agents.mkdir(parents=True) + (agents / "repro-agent.agent.md").write_text( + "---\nname: repro-agent\ndescription: A repro agent.\n---\n\nBody.\n", + encoding="utf-8", + ) + if linked: + (source / ".apm").mkdir() + (source / ".apm" / "agents").symlink_to("../agents", target_is_directory=True) + diagnostics = DiagnosticCollector() + integrators = IntegratorBundle( + prompt=PromptIntegrator(), + agent=AgentIntegrator(), + command=CommandIntegrator(), + instruction=InstructionIntegrator(), + hook=HookIntegrator(), + skill=SkillIntegrator(), + ) + targets = [KNOWN_TARGETS["claude"], KNOWN_TARGETS["copilot"]] + if own_project: + result = integrate_local_content( + project, + targets=targets, + prompt_integrator=integrators.prompt, + agent_integrator=integrators.agent, + command_integrator=integrators.command, + instruction_integrator=integrators.instruction, + hook_integrator=integrators.hook, + skill_integrator=integrators.skill, + diagnostics=diagnostics, + force=False, + managed_files=set(), + ) + else: + package = PackageInfo( + package=APMPackage(name="bundle", version="0.0.0", source="github"), + install_path=source, + package_type=PackageType.APM_PACKAGE, + ) + result = integrate_package_primitives( + package, + project, + targets=targets, + integrators=integrators, + force=False, + managed_files=set(), + diagnostics=diagnostics, + package_name="owner/bundle", + ) + + expected = ( + [] + if linked + else [ + ".claude/agents/repro-agent.md", + ".github/agents/repro-agent.agent.md", + ] + ) + assert result["agents"] == len(expected) + assert sorted(result["deployed_files"]) == expected + assert ( + sorted( + path.relative_to(project).as_posix() + for target in (".claude", ".github") + for path in (project / target).rglob("*") + if path.is_file() + ) + == expected + ) + if linked: + warnings = diagnostics.by_category().get("warning", []) + assert len(warnings) == 1 + assert warnings[0].package == ("_local" if own_project else "owner/bundle") + assert "Skipped symlinked agent source: .apm/agents" in warnings[0].message + assert "Symlinked agent sources are not deployed." in warnings[0].message + assert "real files" in warnings[0].message + assert "apm install" in warnings[0].message + diagnostics.render_summary() + output = capsys.readouterr().out + assert "Skipped symlinked agent source: .apm/agents" in output + assert "real files" in output + else: + assert diagnostics.by_category().get("warning", []) == [] + + +@pytest.mark.parametrize( + ("relative", "directory"), + [ + ("linked.agent.md", False), + (".apm/agents/linked.md", False), + (".apm/agents/nested.md", True), + (".apm", True), + ], +) +@pytest.mark.parametrize("destination", ["contained", "external", "dangling"]) +def test_plan_reports_rejected_links_without_traversing_them( + tmp_path: Path, relative: str, directory: bool, destination: str +) -> None: + """Link rejection retains lexical paths and never authorizes link targets.""" + source = tmp_path / "package" + source.mkdir() + target = (source if destination == "contained" else tmp_path) / "destination" + if destination != "dangling": + if directory: + (target / "agents").mkdir(parents=True) + (target / "agents" / "hidden.agent.md").write_text("Body.\n", encoding="utf-8") + else: + target.write_text("Body.\n", encoding="utf-8") + link = source / relative + link.parent.mkdir(parents=True, exist_ok=True) + link.symlink_to(target, target_is_directory=directory) + (source / "real.agent.md").write_text("Body.\n", encoding="utf-8") + diagnostics = DiagnosticCollector() + package = PackageInfo( + package=APMPackage(name="bundle", version="0.0.0"), + install_path=source, + package_type=PackageType.APM_PACKAGE, + ) + import os + + with patch("apm_cli.install.deployable_source_plan.os.walk", wraps=os.walk) as walk: + plan = DeployableSourcePlan.create( + package, + [KNOWN_TARGETS["claude"], KNOWN_TARGETS["copilot"]], + skill_subset=None, + hooks_approved=False, + canvas_approved=False, + skip_bin=True, + diagnostics=diagnostics, + package_name="owner/bundle", + ) + + assert plan.paths == frozenset({"real.agent.md"}) + assert all(call.args[0] not in (link, target) for call in walk.call_args_list) + warnings = diagnostics.by_category()["warning"] + assert len(warnings) == 1 + expected = ".apm/agents" if relative == ".apm" else relative + assert warnings[0].message.startswith(f"Skipped symlinked agent source: {expected}.") + assert warnings[0].package == "owner/bundle" + + +@pytest.mark.parametrize("agents_enabled", [True, False]) +@pytest.mark.windows_compat +def test_agent_skip_diagnostics_are_target_gated_and_printable( + tmp_path: Path, agents_enabled: bool +) -> None: + """Unselected agents stay quiet; selected source labels cannot inject output.""" + name = "linked-\u00e9.agent.md" + (tmp_path / name).symlink_to(tmp_path / "missing") + diagnostics = DiagnosticCollector() + package = PackageInfo( + package=APMPackage(name="bundle", version="0.0.0"), + install_path=tmp_path, + package_type=PackageType.APM_PACKAGE, + ) + plan = DeployableSourcePlan.create( + package, + [KNOWN_TARGETS["claude"]] if agents_enabled else [], + skill_subset=None, + hooks_approved=False, + canvas_approved=False, + skip_bin=True, + diagnostics=diagnostics, + package_name="bundle\n\u00e9", + ) + assert plan.paths == frozenset() + warnings = diagnostics.by_category().get("warning", []) + assert len(warnings) == int(agents_enabled) + if agents_enabled: + assert warnings[0].message.startswith("Skipped symlinked agent source: linked-?.agent.md.") + assert warnings[0].package == "bundle??" + assert all(0x20 <= ord(char) <= 0x7E for char in warnings[0].message)