From bf994922d7bde809a3f6a47d61901034951702e8 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 19 Oct 2026 15:58:38 +0200 Subject: [PATCH 1/9] fix(hooks): admit and stage hook-only Git-path packages Preserve validated hook layouts through resolver admission and stage replacement updates before touching the installed source. Cover both layouts with real install, restore, update, consent, target and removal lifecycle regression tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82e9592a-494f-4318-864b-5f8e1877372f --- .../content/docs/reference/package-types.md | 41 ++- .../skills/apm-usage/package-authoring.md | 8 + .../marketplace_catalog_and_contract.py | 52 ++++ src/apm_cli/deps/apm_resolver.py | 13 +- src/apm_cli/drift.py | 12 +- src/apm_cli/install/phases/resolve.py | 1 + src/apm_cli/models/validation.py | 52 +++- ...est_architecture_hook_package_admission.py | 57 ++++ .../test_hook_path_package_lifecycle.py | 258 ++++++++++++++++++ .../unit/deps/test_hook_package_admission.py | 91 ++++++ 10 files changed, 556 insertions(+), 29 deletions(-) create mode 100644 tests/integration/test_architecture_hook_package_admission.py create mode 100644 tests/integration/test_hook_path_package_lifecycle.py create mode 100644 tests/unit/deps/test_hook_package_admission.py diff --git a/docs/src/content/docs/reference/package-types.md b/docs/src/content/docs/reference/package-types.md index d81d11d1ce..c40df1b611 100644 --- a/docs/src/content/docs/reference/package-types.md +++ b/docs/src/content/docs/reference/package-types.md @@ -14,7 +14,7 @@ Pick the form that matches the author's intent -- APM preserves it. | `.apm/` (with or without apm.yml) | "I have N independent primitives" | Hoist each primitive into the target's runtime dirs | | `SKILL.md` (alone or with apm.yml -- HYBRID) | "I am one skill bundle" | Copy the whole bundle to `/skills//` | | `skills//SKILL.md` (nested) | "I ship many skills in one repo" | Promote each nested skill to `/skills//` | -| `hooks/*.json` only (no apm.yml or SKILL.md) | "I ship a set of harness hooks" | Deploy each hook to the target's `hooks/` directory | +| `hooks/*.json` or `.apm/hooks/*.json` only (no apm.yml or SKILL.md) | "I ship a set of harness hooks" | Register hooks and referenced resources for the selected targets | | `plugin.json` (no `$schema`, or unrecognized `$schema`) / `.claude-plugin/` | Claude plugin collection | Dissect via plugin artifact mapping | | `plugin.json` with the recognized Agent Plugins `$schema` | Portable Agent Plugin | Installed whole and registered when the effective targets include Copilot | | Marketplace entry with inline `lspServers` or `mcpServers` | Catalog owns server metadata | Synthesize and validate `apm.yml`, then deploy servers | @@ -180,23 +180,36 @@ primitives. in one repository (e.g. all Azure skills, all Firebase skills). Consumers can install the full set or cherry-pick with `--skill`. -## Hook package (`hooks/*.json` only) +## Hook package (`hooks/*.json` or `.apm/hooks/*.json`) -A package whose root contains `hooks/*.json` files but no `apm.yml`, -`SKILL.md`, or `plugin.json`. APM treats the whole directory as a hook -bundle: each hook JSON is deployed to the target's runtime hooks -directory. +A package containing `hooks/*.json` or `.apm/hooks/*.json` but no +`apm.yml`, `SKILL.md`, or `plugin.json` needs no generated manifest or +placeholder skill. Use a Git dependency's `path` to select the package +directory, not its inner hooks directory: -``` -my-hooks/ -+-- hooks/ - +-- pre-commit.json - +-- post-merge.json +```yaml +dependencies: + apm: + - git: acme/agent-packages + path: packages/my-hooks + ref: v1.0.0 ``` -**What gets installed:** every file under `hooks/` is deployed to the -target's hooks runtime path (e.g. `.github/hooks/` for Copilot, -`.claude/hooks/` for Claude). +Here, `packages/my-hooks/hooks/lint.json` or +`packages/my-hooks/.apm/hooks/lint.json` declares the hooks. +Discovery is non-recursive; arbitrary directories are not packages. + +**What gets installed:** hook registrations and referenced script bundles +follow the selected targets and existing executable approval rules. Claude +and Codex use their native configuration files; Copilot uses `.github/hooks/`. +Installation does not execute the hooks. See +[hook authoring](../../producer/author-primitives/hooks-and-commands/) +for command paths, resource selection, and target restrictions. + +`apm install` restores the locked revision; `apm update` refreshes it. +Malformed hook JSON or an invalid replacement package fails before activation, +leaving the previous installation, lockfile, and deployment ownership intact. +`apm uninstall` removes the package's managed hooks and resources. **When to choose:** you ship a set of harness hooks with no other primitives. If you also ship skills or instructions, prefer the `.apm/` 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 5f6f9d6a63..3a521435e9 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -129,6 +129,14 @@ Packages can ship hooks (pre/post tool-use scripts) by placing JSON files under `hooks/` or `.apm/hooks/`. Filename-based hook routing (`*--hooks.json` and `hooks-.json`) is deprecated. +Hook-only Git dependencies need neither `apm.yml` nor `SKILL.md`. Set +`path` to the directory containing `hooks/` or `.apm/hooks/`, not the +inner hooks directory. Discovery is non-recursive. `apm install` restores +the locked revision, `apm update` refreshes it, and `apm uninstall` removes +managed hooks and resources. Malformed hook descriptors or invalid +replacement packages leave the prior installation, lock, and ownership +intact. Hook admission does not grant executable approval or run scripts. + APM computes hook reach with one restriction-only intersection: `effective hook targets = project active targets INTERSECT consumer diff --git a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py index f083749da2..2d4edff035 100644 --- a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py +++ b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py @@ -694,6 +694,58 @@ def _check_package_format_precedence(provider: FactsProvider) -> tuple[Violation "Agent Plugin ingress must delegate package-format precedence to the planner", ) ) + findings.extend( + _require_subs( + provider, + inv, + _RID_FORMAT_PRECEDENCE, + "src/apm_cli/deps/apm_resolver.py", + ( + "package_type, _ = detect_package_type(install_path)", + "PackageType.HOOK_PACKAGE,", + "validation = validate_apm_package(", + ), + "Resolver hook admission must use canonical package classification and validation", + ) + ) + findings.extend( + _require_subs( + provider, + inv, + _RID_FORMAT_PRECEDENCE, + _VALIDATION, + ( + "hook_files = HookIntegrator().find_hook_files(package_path)", + "parse_hook_source(json.load(handle))", + "_validate_hook_descriptors(package_path, result)", + ), + "Hook-only validation must reuse hook discovery and the neutral source grammar", + ) + ) + findings.extend( + _require_subs( + provider, + inv, + _RID_FORMAT_PRECEDENCE, + "src/apm_cli/drift.py", + ( + "package_type, _ = detect_package_type(package_path)", + "if package_type is PackageType.HOOK_PACKAGE:", + ), + "Hook update rechecks must use canonical package classification", + ) + ) + findings.extend( + _forbid_scan( + provider, + inv, + _RID_FORMAT_PRECEDENCE, + ("src/apm_cli/deps/apm_resolver.py",), + re.compile(r"\.(?:rglob|glob)\("), + "Resolver must not discover hook layouts independently of package classification", + exempt=False, + ) + ) return tuple(findings) diff --git a/src/apm_cli/deps/apm_resolver.py b/src/apm_cli/deps/apm_resolver.py index 9c8bf27f62..d9fec02607 100644 --- a/src/apm_cli/deps/apm_resolver.py +++ b/src/apm_cli/deps/apm_resolver.py @@ -1085,6 +1085,11 @@ def _should_force_recheck(self, dep_ref: DependencyReference) -> bool: dep_ref, locked_dep, update_refs=self._update_refs, + package_path=( + dep_ref.get_install_path(self._apm_modules_dir) + if self._apm_modules_dir is not None + else None + ), ) def _try_load_dependency_package( @@ -1265,7 +1270,11 @@ def _try_load_dependency_package( ) package_type, _ = detect_package_type(install_path) - if package_type in (PackageType.MARKETPLACE_PLUGIN, PackageType.SKILL_BUNDLE): + if package_type in ( + PackageType.MARKETPLACE_PLUGIN, + PackageType.SKILL_BUNDLE, + PackageType.HOOK_PACKAGE, + ): validation = validate_apm_package( install_path, source_path=dep_source_path, @@ -1281,7 +1290,7 @@ def _try_load_dependency_package( self._raise_downloaded_package_error( downloaded_candidate, dep_ref, - f"Marketplace Plugin validation produced no package metadata: {install_path}", + f"Package validation produced no package metadata: {install_path}", had_existing_install, ) if not validation.package.source: diff --git a/src/apm_cli/drift.py b/src/apm_cli/drift.py index f871682818..5731eea6b1 100644 --- a/src/apm_cli/drift.py +++ b/src/apm_cli/drift.py @@ -50,6 +50,7 @@ import builtins from dataclasses import replace as _dataclass_replace +from pathlib import Path from typing import TYPE_CHECKING, Any from apm_cli.core.host_providers import effective_host_provider_identity @@ -201,19 +202,26 @@ def should_force_ref_recheck( locked_dep: LockedDependency | None, *, update_refs: bool, + package_path: Path | None = None, ) -> bool: """Return whether an existing install path must reach the download owner. This is the single gate shared by the dependency resolver and its download callback. A normal install rechecks only manifest/lock drift. An explicit - update rechecks semver ranges so they can discover a newer matching tag, - while literal revision pins retain their dedicated update semantics. + update rechecks semver ranges and hook-only packages. Hook replacements + must pass staged admission before any live content is replaced. """ if dep_ref.is_local or getattr(dep_ref, "artifactory_prefix", None): return False if locked_dep is None: return True if update_refs: + if package_path is not None: + from apm_cli.models.validation import PackageType, detect_package_type + + package_type, _ = detect_package_type(package_path) + if package_type is PackageType.HOOK_PACKAGE: + return True return getattr(dep_ref, "ref_kind", None) == "semver" return detect_ref_change(dep_ref, locked_dep, update_refs=False) diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index a89462eda0..3fb72dd7ee 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -467,6 +467,7 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): dep_ref, _locked_for_recheck, update_refs=update_refs, + package_path=install_path, ): return install_path replacement_path = staging_session.prepare_replacement(install_path) diff --git a/src/apm_cli/models/validation.py b/src/apm_cli/models/validation.py index c9878f5196..5d4ab54807 100644 --- a/src/apm_cli/models/validation.py +++ b/src/apm_cli/models/validation.py @@ -422,7 +422,7 @@ def validate_apm_package( # Handle hook-only packages (no apm.yml or SKILL.md) if result.package_type == PackageType.HOOK_PACKAGE: - return _validate_hook_package(package_path, result) + return _validate_hook_package(package_path, result, source_path=source_path) # Handle Agent Plugins through their native immutable contract owner. if result.package_type == PackageType.AGENT_PLUGIN: @@ -520,21 +520,50 @@ def _validate_agent_plugin( return result -def _validate_hook_package(package_path: Path, result: ValidationResult) -> ValidationResult: - """Validate a hook-only package and create APMPackage from its metadata. +def _validate_hook_descriptors(package_path: Path, result: ValidationResult) -> None: + """Validate discovered descriptors through the neutral grammar before admission.""" + import json - A hook package has hooks/*.json (or .apm/hooks/*.json) defining hook - handlers per the Claude Code hooks specification, but no apm.yml or SKILL.md. + from ..hook_contract import HookContractError, parse_hook_source + from ..integration.hook_integrator import HookIntegrator + from ..utils.path_security import PathTraversalError, ensure_path_within + from ..utils.paths import portable_relpath - Args: - package_path: Path to the package directory - result: ValidationResult to populate + hook_files = HookIntegrator().find_hook_files(package_path) + if not hook_files: + result.add_error("Hook package has no deployable hook descriptors.") + return + for hook_file in hook_files: + try: + ensure_path_within(hook_file, package_path) + with hook_file.open(encoding="utf-8") as handle: + parse_hook_source(json.load(handle)) + except ( + OSError, + UnicodeError, + json.JSONDecodeError, + HookContractError, + PathTraversalError, + ) as exc: + result.add_error( + f"Invalid hook descriptor {portable_relpath(hook_file, package_path)}: {exc}. " + "Fix the hook JSON and retry installation." + ) - Returns: - ValidationResult: Updated validation result - """ + +def _validate_hook_package( + package_path: Path, + result: ValidationResult, + *, + source_path: Path | None = None, +) -> ValidationResult: + """Admit an existing hook-only layout without synthesizing a source manifest.""" from .apm_package import APMPackage + _validate_hook_descriptors(package_path, result) + if not result.is_valid: + return result + package_name = package_path.name # Create APMPackage from directory name @@ -543,6 +572,7 @@ def _validate_hook_package(package_path: Path, result: ValidationResult) -> Vali version="1.0.0", description=f"Hook package: {package_name}", package_path=package_path, + source_path=source_path, type=PackageContentType.HYBRID, ) result.package = package diff --git a/tests/integration/test_architecture_hook_package_admission.py b/tests/integration/test_architecture_hook_package_admission.py new file mode 100644 index 0000000000..1c708074f6 --- /dev/null +++ b/tests/integration/test_architecture_hook_package_admission.py @@ -0,0 +1,57 @@ +"""Static regression guards for canonical hook-only package admission.""" + +import ast +from pathlib import Path + +import pytest + +from scripts.architecture_linter.runner import run_selected_rules + +pytestmark = pytest.mark.component +ROOT = Path(__file__).resolve().parents[2] +RULE = "marketplace-integrations-package-format-precedence" + + +@pytest.mark.parametrize( + ("path", "old", "new"), + [ + ( + "src/apm_cli/deps/apm_resolver.py", + " PackageType.HOOK_PACKAGE,\n", + "", + ), + ( + "src/apm_cli/deps/apm_resolver.py", + "package_type, _ = detect_package_type(install_path)", + "package_type = PackageType.HOOK_PACKAGE if list(install_path.glob('*.json')) else None", + ), + ( + "src/apm_cli/models/validation.py", + "hook_files = HookIntegrator().find_hook_files(package_path)", + "hook_files = list(package_path.rglob('*.json'))", + ), + ( + "src/apm_cli/models/validation.py", + "parse_hook_source(json.load(handle))", + "json.load(handle)", + ), + ( + "src/apm_cli/models/validation.py", + " _validate_hook_descriptors(package_path, result)\n", + "", + ), + ( + "src/apm_cli/drift.py", + "package_type, _ = detect_package_type(package_path)", + "package_type = None", + ), + ], +) +def test_hook_admission_cannot_bypass_canonical_owners(path: str, old: str, new: str) -> None: + source = (ROOT / path).read_text(encoding="utf-8") + assert source.count(old) == 1 + mutated = source.replace(old, new, 1) + ast.parse(mutated, filename=path) + report = run_selected_rules(ROOT, (RULE,), source_overrides={path: mutated}) + assert report.failures == () + assert any(violation.rule_id == RULE for violation in report.violations) diff --git a/tests/integration/test_hook_path_package_lifecycle.py b/tests/integration/test_hook_path_package_lifecycle.py new file mode 100644 index 0000000000..8bded33f8b --- /dev/null +++ b/tests/integration/test_hook_path_package_lifecycle.py @@ -0,0 +1,258 @@ +"""Installed-CLI lifecycle for the existing manifestless Git-path hook layouts.""" + +from __future__ import annotations + +import json +import os +from pathlib import Path, PurePosixPath + +import pytest + +from apm_cli.deps.lockfile import LockFile +from apm_cli.models.dependency.reference import DependencyReference +from apm_cli.utils.yaml_io import dump_yaml, load_yaml +from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner, CommandResult +from tests.utils.isolated_apm_environment import IsolatedApmEnvironment +from tests.utils.lifecycle_state import LifecycleStateSnapshot +from tests.utils.local_git_repository import LocalGitRepositoryFactory +from tests.utils.local_package import LocalPackageFactory + +pytestmark = [pytest.mark.e2e, pytest.mark.lifecycle_smoke, pytest.mark.requires_apm_binary] + +_REMOTE = "https://gitlab.example.invalid/acme/hook-packages.git" +_SUBPATH = "hooks/markdown-lint" +_TARGETS = ("claude", "codex") +_CONFIGS = (PurePosixPath(".claude/settings.json"), PurePosixPath(".codex/hooks.json")) +_INSTALL = ("install", "--no-policy", "--parallel-downloads", "0") +_UPDATE = ("update", "--yes", "--parallel-downloads", "0") + + +def _run( + runner: ApmLifecycleRunner, + project: Path, + environment: dict[str, str], + args: tuple[str, ...], + scenario: str, + *, + success: bool = True, +) -> CommandResult: + result = runner.run(args, scenario_id=scenario, cwd=project, env=environment) + assert (result.returncode == 0) is success, result.stdout + result.stderr + return result + + +def _snapshot(project: Path) -> LifecycleStateSnapshot: + return LifecycleStateSnapshot.capture(project, targets=_TARGETS, config_paths=_CONFIGS) + + +def _assert_hooks( + project: Path, marker: str, configs: tuple[PurePosixPath, ...] = _CONFIGS +) -> None: + for relative in configs: + payload = json.loads((project / relative).read_text(encoding="utf-8")) + handlers = payload["hooks"]["PostToolUse"] + assert len(handlers) == 1 + handler = handlers[0] + assert handler["matcher"] == "Write" + command = handler["hooks"][0]["command"] + assert ".apm-resolution-staging" not in command + script = command.removeprefix("python ").strip('"') + script = script.replace("${CLAUDE_PROJECT_DIR}", str(project)) + script = script.replace("${CODEX_PROJECT_DIR}", str(project)) + script_path = Path(script) + if not script_path.is_absolute(): + script_path = project / script_path + assert script_path.is_file(), command + assert marker in script_path.read_text(encoding="utf-8") + assert (script_path.parent / "resource.txt").read_text(encoding="utf-8") == marker + assert not (project / "hook-ran.txt").exists() + assert not (project / ".github/hooks").exists() + lock = LockFile.read(project / "apm.lock.yaml") + assert lock is not None + assert len(lock.dependencies) == 1 + assert lock.deployment_ledger.records + assert all(".apm-resolution-staging" not in key for key in lock.deployment_ledger.records) + + +@pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) +def test_hook_path_install_restore_update_remove( + tmp_path: Path, apm_binary_path: Path, layout: str +) -> None: + isolated = IsolatedApmEnvironment.create(tmp_path / "scenario", base_env=dict(os.environ)) + environment = isolated.subprocess_env() + repositories = LocalGitRepositoryFactory(isolated.repository_root, env=environment) + repository = repositories.create("hook-packages") + package = repository.worktree / _SUBPATH + hooks = package / layout + hooks.mkdir(parents=True) + descriptor = hooks / "lint.json" + descriptor.write_text( + json.dumps( + { + "hooks": { + "PostToolUse": [ + { + "matcher": "Write", + "hooks": [ + { + "type": "command", + "command": f'python "${{CLAUDE_PLUGIN_ROOT}}/{layout}/run.py"', + } + ], + } + ] + } + } + ), + encoding="utf-8", + ) + valid_descriptor = descriptor.read_bytes() + script = hooks / "run.py" + script.write_text( + 'from pathlib import Path\nPath("hook-ran.txt").write_text("v1")\n', + encoding="utf-8", + ) + resource = hooks / "resource.txt" + resource.write_text("v1", encoding="utf-8") + controls = repository.worktree / "controls" + skill = controls / "skill" / "SKILL.md" + skill.parent.mkdir(parents=True) + skill.write_text( + "---\nname: control-skill\ndescription: Valid non-hook control\n---\n# Control\n", + encoding="utf-8", + ) + arbitrary = controls / "arbitrary" / "notes.txt" + arbitrary.parent.mkdir(parents=True) + arbitrary.write_text("Not a package\n", encoding="utf-8") + nested = controls / "nested" / "other" / "hooks" / "lint.json" + nested.parent.mkdir(parents=True) + nested.write_bytes(valid_descriptor) + malformed = controls / "malformed" / "hooks" / "lint.json" + malformed.parent.mkdir(parents=True) + malformed.write_text("{", encoding="utf-8") + first = repositories.commit(repository, message="seed manifestless hooks") + repositories.install_url_rewrite(repository, _REMOTE) + dependency = {"git": _REMOTE, "type": "gitlab", "path": _SUBPATH, "ref": "main"} + consumers = LocalPackageFactory(isolated.work_root) + consumer = consumers.create("consumer", dependencies=(dependency,), targets=_TARGETS) + runner = ApmLifecycleRunner((str(apm_binary_path),), scenario_timeout_seconds=300) + ref = DependencyReference.parse_from_dict(dependency) + installed = ref.get_install_path(consumer.root / "apm_modules") + + _run(runner, consumer.root, environment, _INSTALL, "hook-path-fresh") + _assert_hooks(consumer.root, "v1") + assert not (package / "apm.yml").exists() + assert not (installed / "apm.yml").exists() + assert not (installed / "SKILL.md").exists() + initial_lock = (consumer.root / "apm.lock.yaml").read_bytes() + initial_state = _snapshot(consumer.root) + _run(runner, consumer.root, environment, _INSTALL, "hook-path-reinstall") + assert _snapshot(consumer.root).semantic_bytes == initial_state.semantic_bytes + + script.write_text(script.read_text(encoding="utf-8").replace("v1", "v2"), encoding="utf-8") + resource.write_text("v2", encoding="utf-8") + second = repositories.commit(repository, message="update hook and sibling resource") + _run(runner, consumer.root, environment, _UPDATE, "hook-path-update") + _assert_hooks(consumer.root, "v2") + assert first.sha != second.sha + assert second.sha in (consumer.root / "apm.lock.yaml").read_text(encoding="utf-8") + + restored = consumers.create("restored", dependencies=(dependency,), targets=_TARGETS) + (restored.root / "apm.lock.yaml").write_bytes(initial_lock) + restored_install = ref.get_install_path(restored.root / "apm_modules") + assert not restored_install.exists() + _run(runner, restored.root, environment, _INSTALL, "hook-path-locked-restore") + _assert_hooks(restored.root, "v1") + assert first.sha in (restored.root / "apm.lock.yaml").read_text(encoding="utf-8") + + for name, replacement in ( + ("malformed-json", b"{"), + ("invalid-grammar", b'{"hooks":{"PostToolUse":[{"command":42}]}}'), + ("arbitrary-directory", None), + ): + before = _snapshot(consumer.root) + source_before = { + path.relative_to(installed).as_posix(): path.read_bytes() + for path in installed.rglob("*") + if path.is_file() + } + if replacement is None: + descriptor.unlink() + else: + descriptor.write_bytes(replacement) + repositories.commit(repository, message=name) + failed = _run(runner, consumer.root, environment, _UPDATE, name, success=False) + assert "invalid" in (failed.stdout + failed.stderr).lower() + after = _snapshot(consumer.root) + assert after.lockfile_bytes == before.lockfile_bytes + assert after.manifest_bytes == before.manifest_bytes + assert after.deployment_records == before.deployment_records + assert after.files == before.files + assert installed.exists(), f"{name}: {failed.stdout}\n{failed.stderr}" + assert { + path.relative_to(installed).as_posix(): path.read_bytes() + for path in installed.rglob("*") + if path.is_file() + } == source_before + _assert_hooks(consumer.root, "v2") + + descriptor.write_bytes(valid_descriptor) + repositories.commit(repository, message="repair source") + gated = consumers.create("gated", dependencies=(dependency,), targets=_TARGETS) + manifest = load_yaml(gated.manifest_path) + manifest["allowExecutables"] = {} + dump_yaml(manifest, gated.manifest_path) + _run(runner, gated.root, environment, _INSTALL, "hook-path-unapproved") + assert all(not (gated.root / config).exists() for config in _CONFIGS) + assert not (gated.root / "hook-ran.txt").exists() + gated_lock = LockFile.read(gated.root / "apm.lock.yaml") + assert next(iter(gated_lock.dependencies.values())).exec_status == "gated_pending_approval" + _run( + runner, + gated.root, + environment, + ("approve", ref.get_unique_key()), + "hook-path-approve", + ) + _run(runner, gated.root, environment, _INSTALL, "hook-path-approved") + _assert_hooks(gated.root, "v2") + + restricted = consumers.create( + "restricted", + dependencies=({**dependency, "targets": ["claude"]},), + targets=_TARGETS, + ) + _run(runner, restricted.root, environment, _INSTALL, "hook-path-restricted") + _assert_hooks(restricted.root, "v2", (_CONFIGS[0],)) + assert not (restricted.root / ".codex/hooks.json").exists() + + _run(runner, consumer.root, environment, ("uninstall", ref.to_canonical()), "hook-path-remove") + assert not installed.exists() + assert not (consumer.root / "apm.lock.yaml").exists() + for target in _TARGETS: + assert not (consumer.root / f".{target}/hooks/markdown-lint").exists() + for config in _CONFIGS: + path = consumer.root / config + if path.exists(): + assert not json.loads(path.read_text(encoding="utf-8")).get("hooks") + assert not _snapshot(consumer.root).deployment_records + + for name in ("skill", "arbitrary", "nested", "malformed"): + control_dependency = {**dependency, "path": f"controls/{name}"} + control = consumers.create( + f"control-{name}", dependencies=(control_dependency,), targets=("copilot",) + ) + _run( + runner, + control.root, + environment, + _INSTALL, + f"hook-path-control-{name}", + success=name == "skill", + ) + if name == "skill": + assert (control.root / ".agents/skills/skill/SKILL.md").is_file() + else: + assert not (control.root / "apm.lock.yaml").exists() + control_ref = DependencyReference.parse_from_dict(control_dependency) + assert not control_ref.get_install_path(control.root / "apm_modules").exists() diff --git a/tests/unit/deps/test_hook_package_admission.py b/tests/unit/deps/test_hook_package_admission.py new file mode 100644 index 0000000000..4938875699 --- /dev/null +++ b/tests/unit/deps/test_hook_package_admission.py @@ -0,0 +1,91 @@ +"""Hook-only candidates must pass canonical validation before activation.""" + +from pathlib import Path +from unittest.mock import Mock + +import pytest + +from apm_cli.deps.apm_resolver import APMDependencyResolver, DownloadedPackageError +from apm_cli.deps.lockfile import LockedDependency +from apm_cli.drift import should_force_ref_recheck +from apm_cli.models.dependency.reference import DependencyReference +from apm_cli.models.validation import PackageType, validate_apm_package + +pytestmark = pytest.mark.component + +_VALID_HOOK = '{"hooks":{"preToolUse":[{"bash":"echo fixture"}]}}' + + +@pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) +def test_hook_package_uses_validated_metadata_before_activation( + tmp_path: Path, layout: str +) -> None: + candidate = tmp_path / "candidate" + hooks = candidate / layout + hooks.mkdir(parents=True) + (hooks / "lint.json").write_text(_VALID_HOOK, encoding="utf-8") + ref = DependencyReference(repo_url="acme/hooks", virtual_path="lint", is_virtual=True) + validation = validate_apm_package(candidate) + assert validation.is_valid + assert validation.package_type is PackageType.HOOK_PACKAGE + activation = Mock(return_value=candidate) + resolver = APMDependencyResolver( + apm_modules_dir=tmp_path / "modules", + download_callback=Mock(return_value=candidate), + activation_callback=activation, + ) + + package = resolver._try_load_dependency_package(ref) + + assert package is not None + assert package.description == validation.package.description + assert package.source == ref.repo_url + assert package.package_path == candidate + assert package.source_path == candidate + activation.assert_called_once_with(candidate) + assert not (candidate / "apm.yml").exists() + assert not (candidate / "SKILL.md").exists() + + +@pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) +@pytest.mark.parametrize( + "content", + ["{", "[]", '{"hooks":[]}', '{"hooks":{"preToolUse":[{"bash":42}]}}'], +) +def test_malformed_hook_candidate_never_activates( + tmp_path: Path, layout: str, content: str +) -> None: + candidate = tmp_path / "candidate" + hooks = candidate / layout + hooks.mkdir(parents=True) + (hooks / "lint.json").write_text(content, encoding="utf-8") + validation = validate_apm_package(candidate) + assert not validation.is_valid + assert validation.package is None + assert "lint.json" in "; ".join(validation.errors) + activation = Mock() + resolver = APMDependencyResolver( + apm_modules_dir=tmp_path / "modules", + download_callback=Mock(return_value=candidate), + activation_callback=activation, + ) + + with pytest.raises(DownloadedPackageError, match=r"lint\.json"): + resolver._try_load_dependency_package(DependencyReference(repo_url="acme/hooks")) + + activation.assert_not_called() + + +@pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) +def test_hook_updates_reenter_staged_admission(tmp_path: Path, layout: str) -> None: + hooks = tmp_path / layout + hooks.mkdir(parents=True) + (hooks / "lint.json").write_text(_VALID_HOOK, encoding="utf-8") + ref = DependencyReference(repo_url="acme/hooks", reference="main") + locked = LockedDependency(repo_url="acme/hooks", resolved_ref="main", resolved_commit="a" * 40) + + assert should_force_ref_recheck(ref, locked, update_refs=True, package_path=tmp_path) + assert not should_force_ref_recheck(ref, locked, update_refs=False, package_path=tmp_path) + + (tmp_path / "SKILL.md").write_text("---\nname: control\n---\n# Skill\n", encoding="utf-8") + assert not should_force_ref_recheck(ref, locked, update_refs=True, package_path=tmp_path) From 78b9af3dd9affc687650af0464d56be914cd39b6 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 19 Oct 2026 16:13:27 +0200 Subject: [PATCH 2/9] docs(changelog): record hook package lifecycle repair (#3282) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82e9592a-494f-4318-864b-5f8e1877372f --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c230709888..dbcd63e6c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Hook-only Git-path packages using `hooks/*.json` or `.apm/hooks/*.json` install without a placeholder manifest or skill, and invalid updates preserve the installed source, lockfile and deployment ownership. (#3282) - `apm install` and `apm update` no longer fail with `Object of type mappingproxy is not JSON serializable` when a hook carries nested metadata such as `env`; Claude, Codex, Gemini and Antigravity hook files keep that metadata as plain JSON. (by @abhishekacharya200, with regression coverage from @MohammedAlkindi in #3199, closes #3167) (#3211) ## [0.34.0] - 2026-10-16 From 94ab4fc3c54f173f0e2ac3be3c59585695c3485e Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 19 Oct 2026 17:18:12 +0200 Subject: [PATCH 3/9] fix: preserve scoped hook updates and validated replacement reuse Keep selective refreshes inside their selected dependency subtree; bind callback reuse to validated source bytes and the published path. Cover rejected replacements, shared parser and scope mutations, and repair valid-hook CI fixtures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a --- .../content/docs/reference/package-types.md | 2 + .../skills/apm-usage/package-authoring.md | 3 +- .../marketplace_catalog_and_contract.py | 26 ++++ src/apm_cli/deps/apm_resolver.py | 13 ++ src/apm_cli/drift.py | 7 +- src/apm_cli/install/context.py | 1 + src/apm_cli/install/phases/integrate.py | 21 +-- src/apm_cli/install/phases/resolve.py | 26 ++-- src/apm_cli/install/sources.py | 7 +- tests/conftest.py | 12 ++ ...est_architecture_hook_package_admission.py | 30 ++++ .../test_hook_path_package_lifecycle.py | 147 ++++++++++++++++-- .../unit/deps/test_hook_package_admission.py | 89 ++++++++++- tests/unit/test_models_validation_phase3.py | 10 +- tests/unit/test_models_validation_rules.py | 10 +- 15 files changed, 353 insertions(+), 51 deletions(-) diff --git a/docs/src/content/docs/reference/package-types.md b/docs/src/content/docs/reference/package-types.md index c40df1b611..36430d8b40 100644 --- a/docs/src/content/docs/reference/package-types.md +++ b/docs/src/content/docs/reference/package-types.md @@ -207,6 +207,8 @@ Installation does not execute the hooks. See for command paths, resource selection, and target restrictions. `apm install` restores the locked revision; `apm update` refreshes it. +Updating selected packages leaves unrelated installed hook packages unchanged, +even when their upstream descriptors have become invalid. Malformed hook JSON or an invalid replacement package fails before activation, leaving the previous installation, lockfile, and deployment ownership intact. `apm uninstall` removes the package's managed hooks and resources. 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 220bbbced9..c984a90749 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -10,7 +10,7 @@ how to install it: | `.apm/` (with or without apm.yml) | Multiple independent primitives | Hoist each primitive into the consumer runtime dirs | | `SKILL.md` (alone, or with apm.yml = HYBRID) | One skill bundle | Copy whole tree to `/skills//` | | `skills//SKILL.md` | Many skills in one repo | Promote each nested skill to `/skills//` | -| `hooks/*.json` only | Harness hook package | Deploy hooks to the target's hooks directory | +| `hooks/*.json` or `.apm/hooks/*.json` only | Harness hook package | Register hooks and referenced resources for the selected targets | | `plugin.json` (no `$schema`, or unrecognized `$schema`) / `.claude-plugin/` | Claude plugin collection | Dissect via plugin artifact mapping | | `plugin.json` with the recognized Agent Plugins `$schema` | Portable Agent Plugin | Acquired and locked as one opaque unit; registered when effective targets include Copilot and admission gates pass. Excluded targets create no native registration or loose primitive projection. APM does not require the runtime during lifecycle operations; loading requires supported Copilot CLI 1.0.81 or newer | @@ -136,6 +136,7 @@ the locked revision, `apm update` refreshes it, and `apm uninstall` removes managed hooks and resources. Malformed hook descriptors or invalid replacement packages leave the prior installation, lock, and ownership intact. Hook admission does not grant executable approval or run scripts. +Updating selected packages does not replace unrelated installed hook packages. APM computes hook reach with one restriction-only intersection: diff --git a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py index 2d4edff035..c354987c04 100644 --- a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py +++ b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py @@ -704,6 +704,8 @@ def _check_package_format_precedence(provider: FactsProvider) -> tuple[Violation "package_type, _ = detect_package_type(install_path)", "PackageType.HOOK_PACKAGE,", "validation = validate_apm_package(", + "update_keys=self._update_keys,", + "self._update_keys.add(dep_ref.get_unique_key())", ), "Resolver hook admission must use canonical package classification and validation", ) @@ -731,10 +733,34 @@ def _check_package_format_precedence(provider: FactsProvider) -> tuple[Violation ( "package_type, _ = detect_package_type(package_path)", "if package_type is PackageType.HOOK_PACKAGE:", + "if update_keys is not None and dep_ref.get_unique_key() not in update_keys:", ), "Hook update rechecks must use canonical package classification", ) ) + for path, snippets in ( + ( + "src/apm_cli/install/phases/resolve.py", + ("update_keys=update_keys,", "content_hash = compute_package_hash(candidate_path)"), + ), + ( + "src/apm_cli/install/phases/integrate.py", + ( + "receipt[0] == install_path.resolve()", + "if verify_package_hash(install_path, receipt[1]):", + ), + ), + ): + findings.extend( + _require_subs( + provider, + inv, + _RID_FORMAT_PRECEDENCE, + path, + snippets, + "Hook replacement reuse must preserve selected scope and validated path/hash evidence", + ) + ) findings.extend( _forbid_scan( provider, diff --git a/src/apm_cli/deps/apm_resolver.py b/src/apm_cli/deps/apm_resolver.py index d9fec02607..154b58a075 100644 --- a/src/apm_cli/deps/apm_resolver.py +++ b/src/apm_cli/deps/apm_resolver.py @@ -107,6 +107,7 @@ def __init__( cache_validation_callback: Callable[[Path, str], Path | None] | None = None, reference_resolver: ReferenceResolver | None = None, frozen: bool = False, + update_keys: set[str] | None = None, ): """Initialize the resolver with maximum recursion depth. @@ -130,6 +131,8 @@ def __init__( install path already exists, at any depth -- see that method's docstring for why this can't be scoped to direct deps only. + update_keys: Shared selected-package keys for scoped updates. + Selected descendants are added before download callbacks run. existing_lockfile: Existing resolved dependency state used by the canonical ref-drift owner to decide whether a plain install must re-enter the download callback. @@ -145,6 +148,7 @@ def __init__( self._activation_callback = activation_callback self._cache_validation_callback = cache_validation_callback self._update_refs = update_refs + self._update_keys = update_keys self._existing_lockfile = existing_lockfile self._reference_resolver = reference_resolver self._frozen = frozen @@ -1045,6 +1049,14 @@ def _load_work_item(self, item): loop-iteration variable. """ node, dep_ref, parent_node, _is_dev = item + if self._update_keys is not None: + ancestor = parent_node + while ancestor is not None: + if ancestor.dependency_ref.get_unique_key() in self._update_keys: + with self._download_lock: + self._update_keys.add(dep_ref.get_unique_key()) + break + ancestor = ancestor.parent # Compute breadcrumb chain from this node's ancestry so download # errors can report "root > mid > failing-dep" context. parent_chain = node.get_ancestor_chain() @@ -1085,6 +1097,7 @@ def _should_force_recheck(self, dep_ref: DependencyReference) -> bool: dep_ref, locked_dep, update_refs=self._update_refs, + update_keys=self._update_keys, package_path=( dep_ref.get_install_path(self._apm_modules_dir) if self._apm_modules_dir is not None diff --git a/src/apm_cli/drift.py b/src/apm_cli/drift.py index 5731eea6b1..da73635c2a 100644 --- a/src/apm_cli/drift.py +++ b/src/apm_cli/drift.py @@ -203,6 +203,7 @@ def should_force_ref_recheck( *, update_refs: bool, package_path: Path | None = None, + update_keys: set[str] | None = None, ) -> bool: """Return whether an existing install path must reach the download owner. @@ -216,13 +217,17 @@ def should_force_ref_recheck( if locked_dep is None: return True if update_refs: + if update_keys is not None and dep_ref.get_unique_key() not in update_keys: + return False + if getattr(dep_ref, "ref_kind", None) == "semver": + return True if package_path is not None: from apm_cli.models.validation import PackageType, detect_package_type package_type, _ = detect_package_type(package_path) if package_type is PackageType.HOOK_PACKAGE: return True - return getattr(dep_ref, "ref_kind", None) == "semver" + return False return detect_ref_change(dep_ref, locked_dep, update_refs=False) diff --git a/src/apm_cli/install/context.py b/src/apm_cli/install/context.py index d674424281..701d273834 100644 --- a/src/apm_cli/install/context.py +++ b/src/apm_cli/install/context.py @@ -158,6 +158,7 @@ class InstallContext: # never backfilled with a sentinel. package_declared_licenses: dict[str, str] = field(default_factory=dict) content_hash_verified_deps: set[str] = field(default_factory=set) + callback_content_hashes: dict[str, tuple[Path, str]] = field(default_factory=dict) # Deps whose content hash is expected to change legitimately: # populated by _resolve_download_strategy in phases/integrate.py # (branch-ref `remote_drifted` guard and v<=0.12.2 self-heal block), diff --git a/src/apm_cli/install/phases/integrate.py b/src/apm_cli/install/phases/integrate.py index 295b326a37..f8460102dc 100644 --- a/src/apm_cli/install/phases/integrate.py +++ b/src/apm_cli/install/phases/integrate.py @@ -249,16 +249,17 @@ def _resolve_download_strategy( ref_changed=ref_changed, ) - # Verify content integrity when lockfile has a hash. - # NOTE: when _already_resolved_sha_match is True, the callback has already - # written the correct content for the current remote SHA -- but the lockfile - # content_hash still refers to the *previous* content. If the remote content - # changed (which is the typical stale-lockfile scenario), verify_package_hash - # will mismatch, safe_rmtree fires, and skip_download resets to False, causing - # a re-download. This is the correct safety behaviour but means the - # optimisation is a no-op for update scenarios where content_hash is present - # and stale. A follow-up can target this by propagating the callback-downloaded - # content_hash into the verified set before this guard runs. + # A same-SHA callback receipt binds validated bytes to their published path. + # Recheck those bytes rather than comparing them with the previous revision. + receipt = ctx.callback_content_hashes.get(dep_key) + if _already_resolved_sha_match and receipt and receipt[0] == install_path.resolve(): + from apm_cli.utils.content_hash import verify_package_hash + + if verify_package_hash(install_path, receipt[1]): + content_hash_already_verified = True + ctx.content_hash_verified_deps.add(dep_key) + + # Missing, stale or tampered callback evidence retains the lock hash check. if ( skip_download and _dep_locked_chk diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index 3fb72dd7ee..a1153386b2 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -155,8 +155,12 @@ def _activate_validated_candidate( callback_downloaded: dict[str, str | None], callback_lock: Any, tui: Any, + callback_content_hashes: dict[str, tuple[Path, str]] | None = None, ) -> Path: """Publish one accepted candidate, then expose its download metadata.""" + from apm_cli.utils.content_hash import compute_package_hash + + content_hash = compute_package_hash(candidate_path) live_path = staging_session.publish_replacement(candidate_path) with callback_lock: pending = pending_downloads.pop(candidate_path, None) @@ -164,6 +168,8 @@ def _activate_validated_candidate( raise RuntimeError(f"Validated candidate has no download record: {candidate_path}") dep_key, resolved_sha = pending callback_downloaded[dep_key] = resolved_sha + if callback_content_hashes is not None: + callback_content_hashes[dep_key] = (live_path.resolve(), content_hash) if tui is not None: tui.task_completed(dep_key) return live_path @@ -418,6 +424,7 @@ def _resolve_dependencies( # lockfile entries for packages not in the manifest, unlike --update # which may restructure the whole graph). update_refs = _requires_remote_ref_resolution(ctx) + update_keys = set(ctx.only_packages) if update_refs and ctx.only_packages else None if ctx.refresh and ctx.logger: ctx.logger.verbose_detail("[*] --refresh: bypassing cached package content") logger = ctx.logger @@ -468,6 +475,7 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): _locked_for_recheck, update_refs=update_refs, package_path=install_path, + update_keys=update_keys, ): return install_path replacement_path = staging_session.prepare_replacement(install_path) @@ -746,9 +754,11 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): callback_downloaded=callback_downloaded, callback_lock=callback_lock, tui=getattr(ctx, "tui", None), + callback_content_hashes=ctx.callback_content_hashes, ), auth_resolver=ctx.auth_resolver, update_refs=update_refs, + update_keys=update_keys, existing_lockfile=existing_lockfile, reference_resolver=downloader, frozen=ctx.frozen, @@ -759,18 +769,7 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): ), ) - # Resolver reads ``/apm.yml``. Preserve the original - # ``ctx.apm_dir`` anchor for every non-``--root`` install (zero - # behavior change: USER -> ``~/.apm``, PROJECT -> deploy root == cwd). - # When ``ctx.source_root`` differs from ``ctx.project_root`` (set by - # ``apm install --root`` via the pipeline), the manifest read diverges - # to ``ctx.source_root`` ($PWD) so sources keep resolving from the - # user's working directory while writes land under the deploy root. - # Using the ctx field (rather than the global ContextVar) makes this - # branch reachable for any caller that sets source_root directly. - # ``apm_modules_dir`` is already pinned on the resolver above, so - # this arg selects only where ``apm.yml`` is read -- never where - # ``apm_modules/`` is written. + # The graph reads source_root; apm_modules_dir remains the deployment root. dependency_graph = _build_dependency_graph(ctx, resolver) _fail_on_resolution_errors(ctx, dependency_graph) @@ -899,9 +898,6 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): dep_base_dirs = {} ctx.dep_base_dirs = dep_base_dirs - # ------------------------------------------------------------------ - # Write ancillary state to ctx for later phases - # ------------------------------------------------------------------ ctx.callback_downloaded = callback_downloaded ctx.callback_failures = callback_failures ctx.transitive_failures = transitive_failures diff --git a/src/apm_cli/install/sources.py b/src/apm_cli/install/sources.py index 24af889620..14f939dc14 100644 --- a/src/apm_cli/install/sources.py +++ b/src/apm_cli/install/sources.py @@ -391,8 +391,11 @@ def acquire(self) -> Materialization | None: _ref = _reg_res.version if _reg_res else (dep_ref.reference or "") else: _ref = dep_ref.reference or "" - # Prefer the lockfile SHA, then this run's callback SHA. - _sha = format_short_sha(dep_locked_chk.resolved_commit) if dep_locked_chk else "" + # Fresh callback bytes have their own identity, not the previous lock SHA. + _commit = ctx.callback_downloaded.get(dep_key) if self.fetched_this_run else None + if not _commit and dep_locked_chk: + _commit = dep_locked_chk.resolved_commit + _sha = format_short_sha(_commit) if _commit else "" if not _sha: _callback_sha = ctx.callback_downloaded.get(dep_key) if _callback_sha: diff --git a/tests/conftest.py b/tests/conftest.py index 3093b8bc8f..5ab7fe467a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -29,6 +29,18 @@ import pytest + +@pytest.fixture +def valid_hook_package(tmp_path: Path) -> Path: + """Materialize the minimum valid manifestless hook package.""" + hooks = tmp_path / "hooks" + hooks.mkdir() + (hooks / "lint.json").write_text( + '{"hooks":{"preToolUse":[{"bash":"echo fixture"}]}}', encoding="utf-8" + ) + return tmp_path + + _TMP_HOME = Path(tempfile.mkdtemp(prefix="apm-test-home-")) diff --git a/tests/integration/test_architecture_hook_package_admission.py b/tests/integration/test_architecture_hook_package_admission.py index 1c708074f6..740014726d 100644 --- a/tests/integration/test_architecture_hook_package_admission.py +++ b/tests/integration/test_architecture_hook_package_admission.py @@ -45,6 +45,36 @@ "package_type, _ = detect_package_type(package_path)", "package_type = None", ), + ( + "src/apm_cli/drift.py", + "if update_keys is not None and dep_ref.get_unique_key() not in update_keys:", + "if False:", + ), + ( + "src/apm_cli/deps/apm_resolver.py", + "update_keys=self._update_keys,", + "update_keys=None,", + ), + ( + "src/apm_cli/deps/apm_resolver.py", + "self._update_keys.add(dep_ref.get_unique_key())", + "pass", + ), + ( + "src/apm_cli/install/phases/integrate.py", + "receipt[0] == install_path.resolve()", + "True", + ), + ( + "src/apm_cli/install/phases/integrate.py", + "if verify_package_hash(install_path, receipt[1]):", + "if True:", + ), + ( + "src/apm_cli/install/phases/resolve.py", + "content_hash = compute_package_hash(candidate_path)", + "content_hash = 'unverified'", + ), ], ) def test_hook_admission_cannot_bypass_canonical_owners(path: str, old: str, new: str) -> None: diff --git a/tests/integration/test_hook_path_package_lifecycle.py b/tests/integration/test_hook_path_package_lifecycle.py index 8bded33f8b..f18edef40c 100644 --- a/tests/integration/test_hook_path_package_lifecycle.py +++ b/tests/integration/test_hook_path_package_lifecycle.py @@ -45,6 +45,15 @@ def _snapshot(project: Path) -> LifecycleStateSnapshot: return LifecycleStateSnapshot.capture(project, targets=_TARGETS, config_paths=_CONFIGS) +def _source_bytes(installed: Path) -> dict[str, bytes]: + assert installed.is_dir() + return { + path.relative_to(installed).as_posix(): path.read_bytes() + for path in installed.rglob("*") + if path.is_file() + } + + def _assert_hooks( project: Path, marker: str, configs: tuple[PurePosixPath, ...] = _CONFIGS ) -> None: @@ -152,7 +161,14 @@ def test_hook_path_install_restore_update_remove( script.write_text(script.read_text(encoding="utf-8").replace("v1", "v2"), encoding="utf-8") resource.write_text("v2", encoding="utf-8") second = repositories.commit(repository, message="update hook and sibling resource") - _run(runner, consumer.root, environment, _UPDATE, "hook-path-update") + updated = _run( + runner, + consumer.root, + environment, + _UPDATE, + "hook-path-update", + ) + assert "Content hash mismatch" not in updated.stdout + updated.stderr _assert_hooks(consumer.root, "v2") assert first.sha != second.sha assert second.sha in (consumer.root / "apm.lock.yaml").read_text(encoding="utf-8") @@ -171,11 +187,7 @@ def test_hook_path_install_restore_update_remove( ("arbitrary-directory", None), ): before = _snapshot(consumer.root) - source_before = { - path.relative_to(installed).as_posix(): path.read_bytes() - for path in installed.rglob("*") - if path.is_file() - } + source_before = _source_bytes(installed) if replacement is None: descriptor.unlink() else: @@ -189,11 +201,7 @@ def test_hook_path_install_restore_update_remove( assert after.deployment_records == before.deployment_records assert after.files == before.files assert installed.exists(), f"{name}: {failed.stdout}\n{failed.stderr}" - assert { - path.relative_to(installed).as_posix(): path.read_bytes() - for path in installed.rglob("*") - if path.is_file() - } == source_before + assert _source_bytes(installed) == source_before _assert_hooks(consumer.root, "v2") descriptor.write_bytes(valid_descriptor) @@ -256,3 +264,120 @@ def test_hook_path_install_restore_update_remove( assert not (control.root / "apm.lock.yaml").exists() control_ref = DependencyReference.parse_from_dict(control_dependency) assert not control_ref.get_install_path(control.root / "apm_modules").exists() + + +@pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) +@pytest.mark.parametrize("unselected_malformed", [False, True]) +@pytest.mark.parametrize("via_parent", [False, True]) +def test_selective_hook_update_preserves_unselected_package( + tmp_path: Path, + apm_binary_path: Path, + layout: str, + unselected_malformed: bool, + via_parent: bool, +) -> None: + isolated = IsolatedApmEnvironment.create(tmp_path / "selection", base_env=dict(os.environ)) + environment = isolated.subprocess_env() + repositories = LocalGitRepositoryFactory(isolated.repository_root, env=environment) + repository = repositories.create("hook-packages") + dependencies = [] + descriptors = [] + for name in ("selected", "unselected"): + descriptor = repository.worktree / name / layout / "lint.json" + descriptor.parent.mkdir(parents=True) + descriptor.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "hooks": [ + { + "type": "command", + "command": f'python "${{CLAUDE_PLUGIN_ROOT}}/{layout}/run.py" {name}-v1', + } + ] + } + ] + } + } + ), + encoding="utf-8", + ) + (descriptor.parent / "run.py").write_text("raise SystemExit(0)\n", encoding="utf-8") + descriptors.append(descriptor) + dependencies.append({"git": _REMOTE, "type": "gitlab", "path": name, "ref": "main"}) + selected, unselected = map(DependencyReference.parse_from_dict, dependencies) + requested = selected + if via_parent: + parent = repository.worktree / "parent" + parent.mkdir() + dump_yaml( + {"name": "parent", "version": "1.0.0", "dependencies": {"apm": [dependencies[0]]}}, + parent / "apm.yml", + ) + dependencies[0] = {**dependencies[0], "path": "parent"} + requested = DependencyReference.parse_from_dict(dependencies[0]) + repositories.commit(repository, message="seed two hook packages") + repositories.install_url_rewrite(repository, _REMOTE) + consumer = LocalPackageFactory(isolated.work_root).create( + "consumer", dependencies=tuple(dependencies), targets=("claude",) + ) + runner = ApmLifecycleRunner((str(apm_binary_path),), scenario_timeout_seconds=120) + _run(runner, consumer.root, environment, _INSTALL, "selection-install") + installed = unselected.get_install_path(consumer.root / "apm_modules") + source_before = _source_bytes(installed) + before = LockFile.read(consumer.root / "apm.lock.yaml") + locked_unselected = before.get_dependency(unselected.get_unique_key()) + unselected_before = locked_unselected.to_dict() + ownership_before = { + key: record + for key, record in before.deployment_ledger.records.items() + if unselected.get_unique_key() in record.owners + } + assert ownership_before + files_before = { + path: (consumer.root / path).read_bytes() for path in locked_unselected.deployed_files + } + descriptors[0].write_text( + descriptors[0].read_text(encoding="utf-8").replace("v1", "v2"), encoding="utf-8" + ) + descriptors[1].write_text( + "{" if unselected_malformed else descriptors[1].read_text().replace("v1", "v2"), + encoding="utf-8", + ) + advanced = repositories.commit(repository, message="advance both hook sources") + _run( + runner, + consumer.root, + environment, + ("update", requested.get_display_name(), "--yes", "--parallel-downloads", "0"), + "selection-update", + ) + after = LockFile.read(consumer.root / "apm.lock.yaml") + assert after.get_dependency(selected.get_unique_key()).resolved_commit == advanced.sha + assert after.get_dependency(unselected.get_unique_key()).to_dict() == unselected_before + assert { + key: record + for key, record in after.deployment_ledger.records.items() + if unselected.get_unique_key() in record.owners + } == ownership_before + assert installed.exists() + assert _source_bytes(installed) == source_before + assert {path: (consumer.root / path).read_bytes() for path in files_before} == files_before + selected_path = selected.get_install_path(consumer.root / "apm_modules") + selected_source = _source_bytes(selected_path) + state_before = _snapshot(consumer.root) + descriptors[0].write_text("{", encoding="utf-8") + repositories.commit(repository, message="invalidate selected hook source") + _run( + runner, + consumer.root, + environment, + ("update", requested.get_display_name(), "--yes", "--parallel-downloads", "0"), + "selection-invalid-update", + success=False, + ) + assert _snapshot(consumer.root).semantic_bytes == state_before.semantic_bytes + assert _source_bytes(selected_path) == selected_source + assert _source_bytes(installed) == source_before diff --git a/tests/unit/deps/test_hook_package_admission.py b/tests/unit/deps/test_hook_package_admission.py index 4938875699..9e54a3c1f3 100644 --- a/tests/unit/deps/test_hook_package_admission.py +++ b/tests/unit/deps/test_hook_package_admission.py @@ -1,15 +1,23 @@ """Hook-only candidates must pass canonical validation before activation.""" from pathlib import Path +from threading import Lock from unittest.mock import Mock import pytest from apm_cli.deps.apm_resolver import APMDependencyResolver, DownloadedPackageError -from apm_cli.deps.lockfile import LockedDependency +from apm_cli.deps.lockfile import LockedDependency, LockFile from apm_cli.drift import should_force_ref_recheck +from apm_cli.install.context import InstallContext +from apm_cli.install.phases.integrate import _resolve_download_strategy +from apm_cli.install.phases.resolve import _activate_validated_candidate +from apm_cli.install.resolution_staging import ResolutionStagingSession +from apm_cli.install.sources import make_dependency_source +from apm_cli.models.apm_package import GitReferenceType, ResolvedReference from apm_cli.models.dependency.reference import DependencyReference from apm_cli.models.validation import PackageType, validate_apm_package +from apm_cli.utils.content_hash import compute_package_hash pytestmark = pytest.mark.component @@ -89,3 +97,82 @@ def test_hook_updates_reenter_staged_admission(tmp_path: Path, layout: str) -> N (tmp_path / "SKILL.md").write_text("---\nname: control\n---\n# Skill\n", encoding="utf-8") assert not should_force_ref_recheck(ref, locked, update_refs=True, package_path=tmp_path) + + +@pytest.mark.parametrize("evidence", ["valid", "missing", "wrong-path", "tampered", "stale-sha"]) +def test_published_hook_reuse_requires_bound_integrity(tmp_path: Path, evidence: str) -> None: + ref = DependencyReference(repo_url="acme/hooks", reference="main") + key = ref.get_unique_key() + modules = tmp_path / "apm_modules" + installed = ref.get_install_path(modules) + hooks = installed / "hooks" + hooks.mkdir(parents=True) + (hooks / "lint.json").write_text(_VALID_HOOK, encoding="utf-8") + locked = LockedDependency( + repo_url=ref.repo_url, + resolved_ref="main", + resolved_commit="a" * 40, + content_hash=compute_package_hash(installed), + ) + ctx = InstallContext(project_root=tmp_path, apm_dir=tmp_path, update_refs=True) + ctx.apm_modules_dir = modules + ctx.existing_lockfile = LockFile() + ctx.existing_lockfile.add_dependency(locked) + ctx.targets = ["claude"] + ctx.logger = Mock() + ctx.diagnostics = Mock() + ctx.downloader = Mock() + ctx.dependency_graph = Mock() + ctx.dependency_graph.dependency_tree.get_node.return_value = None + ctx.downloader.resolve_git_reference.return_value = ResolvedReference( + original_ref="main", + ref_type=GitReferenceType.BRANCH, + resolved_commit="c" * 40 if evidence == "stale-sha" else "b" * 40, + ref_name="main", + ) + session = ResolutionStagingSession(modules) + candidate = session.prepare_replacement(installed) + (candidate / "hooks").mkdir(parents=True) + (candidate / "hooks/lint.json").write_text( + _VALID_HOOK.replace("fixture", "updated"), encoding="utf-8" + ) + assert validate_apm_package(candidate).is_valid + _activate_validated_candidate( + candidate, + staging_session=session, + pending_downloads={candidate: (key, "b" * 40)}, + callback_downloaded=ctx.callback_downloaded, + callback_content_hashes=ctx.callback_content_hashes, + callback_lock=Lock(), + tui=None, + ) + if evidence == "missing": + ctx.callback_content_hashes.clear() + elif evidence == "wrong-path": + ctx.callback_content_hashes[key] = (tmp_path, ctx.callback_content_hashes[key][1]) + elif evidence == "tampered": + (installed / "hooks/lint.json").write_text(_VALID_HOOK + "\n", encoding="utf-8") + + resolved, skip, locked_check, changed = _resolve_download_strategy(ctx, ref, installed) + + assert skip is (evidence == "valid") + if skip: + source = make_dependency_source( + ctx, + ref, + installed, + key, + resolved_ref=resolved, + dep_locked_chk=locked_check, + ref_changed=changed, + skip_download=skip, + fetched_this_run=True, + ) + assert source.acquire() is not None + ctx.downloader.download_package.assert_not_called() + ctx.logger.download_complete.assert_called_once_with( + ref.repo_url, ref="main", sha="bbbbbbbb", cached=False + ) + ctx.diagnostics.warn.assert_not_called() + assert session.rollback() == [] + assert compute_package_hash(installed) == locked.content_hash diff --git a/tests/unit/test_models_validation_phase3.py b/tests/unit/test_models_validation_phase3.py index 2257d19a1c..95172575db 100644 --- a/tests/unit/test_models_validation_phase3.py +++ b/tests/unit/test_models_validation_phase3.py @@ -638,22 +638,22 @@ def test_hybrid_dispatched(self, tmp_path: Path) -> None: class TestValidateHookPackage: """Test _validate_hook_package internals.""" - def test_returns_valid_result_with_package(self, tmp_path: Path) -> None: + def test_returns_valid_result_with_package(self, valid_hook_package: Path) -> None: """_validate_hook_package creates an APMPackage and returns valid result.""" from apm_cli.models.validation import _validate_hook_package result: ValidationResult = ValidationResult() - returned: ValidationResult = _validate_hook_package(tmp_path, result) + returned: ValidationResult = _validate_hook_package(valid_hook_package, result) assert returned.is_valid is True assert returned.package is not None - assert returned.package.name == tmp_path.name + assert returned.package.name == valid_hook_package.name - def test_package_version_is_1_0_0(self, tmp_path: Path) -> None: + def test_package_version_is_1_0_0(self, valid_hook_package: Path) -> None: """_validate_hook_package sets package version to '1.0.0'.""" from apm_cli.models.validation import _validate_hook_package result: ValidationResult = ValidationResult() - returned: ValidationResult = _validate_hook_package(tmp_path, result) + returned: ValidationResult = _validate_hook_package(valid_hook_package, result) assert returned.package is not None assert returned.package.version == "1.0.0" diff --git a/tests/unit/test_models_validation_rules.py b/tests/unit/test_models_validation_rules.py index 1604a46823..0b0a54fd80 100644 --- a/tests/unit/test_models_validation_rules.py +++ b/tests/unit/test_models_validation_rules.py @@ -638,22 +638,22 @@ def test_hybrid_dispatched(self, tmp_path: Path) -> None: class TestValidateHookPackage: """Test _validate_hook_package internals.""" - def test_returns_valid_result_with_package(self, tmp_path: Path) -> None: + def test_returns_valid_result_with_package(self, valid_hook_package: Path) -> None: """_validate_hook_package creates an APMPackage and returns valid result.""" from apm_cli.models.validation import _validate_hook_package result: ValidationResult = ValidationResult() - returned: ValidationResult = _validate_hook_package(tmp_path, result) + returned: ValidationResult = _validate_hook_package(valid_hook_package, result) assert returned.is_valid is True assert returned.package is not None - assert returned.package.name == tmp_path.name + assert returned.package.name == valid_hook_package.name - def test_package_version_is_1_0_0(self, tmp_path: Path) -> None: + def test_package_version_is_1_0_0(self, valid_hook_package: Path) -> None: """_validate_hook_package sets package version to '1.0.0'.""" from apm_cli.models.validation import _validate_hook_package result: ValidationResult = ValidationResult() - returned: ValidationResult = _validate_hook_package(tmp_path, result) + returned: ValidationResult = _validate_hook_package(valid_hook_package, result) assert returned.package is not None assert returned.package.version == "1.0.0" From 7d5e32860ef1a0a757d308606870eb42bb0ecd4c Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 19 Oct 2026 17:36:21 +0200 Subject: [PATCH 4/9] fix: recheck shared hook children when selected through a parent Propagate scoped keys while discovering edges and revisit already-loaded shared nodes once when newly selected. Preserve installed source bytes on invalid shared-child updates and cover the scope gate directly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a --- .../skills/apm-usage/package-authoring.md | 2 +- .../marketplace_catalog_and_contract.py | 3 ++- src/apm_cli/deps/apm_resolver.py | 23 ++++++++++++------- ...est_architecture_hook_package_admission.py | 7 +++++- .../test_hook_path_package_lifecycle.py | 14 +++++++---- .../unit/deps/test_hook_package_admission.py | 6 +++++ 6 files changed, 39 insertions(+), 16 deletions(-) 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 c984a90749..17a4d84042 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -10,7 +10,7 @@ how to install it: | `.apm/` (with or without apm.yml) | Multiple independent primitives | Hoist each primitive into the consumer runtime dirs | | `SKILL.md` (alone, or with apm.yml = HYBRID) | One skill bundle | Copy whole tree to `/skills//` | | `skills//SKILL.md` | Many skills in one repo | Promote each nested skill to `/skills//` | -| `hooks/*.json` or `.apm/hooks/*.json` only | Harness hook package | Register hooks and referenced resources for the selected targets | +| `hooks/*.json` or `.apm/hooks/*.json` only | Harness hook package | Add hook commands to each target's configuration and copy referenced files | | `plugin.json` (no `$schema`, or unrecognized `$schema`) / `.claude-plugin/` | Claude plugin collection | Dissect via plugin artifact mapping | | `plugin.json` with the recognized Agent Plugins `$schema` | Portable Agent Plugin | Acquired and locked as one opaque unit; registered when effective targets include Copilot and admission gates pass. Excluded targets create no native registration or loose primitive projection. APM does not require the runtime during lifecycle operations; loading requires supported Copilot CLI 1.0.81 or newer | diff --git a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py index c354987c04..fe675dc9ba 100644 --- a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py +++ b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py @@ -705,7 +705,8 @@ def _check_package_format_precedence(provider: FactsProvider) -> tuple[Violation "PackageType.HOOK_PACKAGE,", "validation = validate_apm_package(", "update_keys=self._update_keys,", - "self._update_keys.add(dep_ref.get_unique_key())", + "self._update_keys.add(sub_dep.get_unique_key())", + "and dep_ref.get_unique_key() not in self._scoped_rechecked", ), "Resolver hook admission must use canonical package classification and validation", ) diff --git a/src/apm_cli/deps/apm_resolver.py b/src/apm_cli/deps/apm_resolver.py index 154b58a075..cdb841435e 100644 --- a/src/apm_cli/deps/apm_resolver.py +++ b/src/apm_cli/deps/apm_resolver.py @@ -149,6 +149,7 @@ def __init__( self._cache_validation_callback = cache_validation_callback self._update_refs = update_refs self._update_keys = update_keys + self._scoped_rechecked: set[str] = set() self._existing_lockfile = existing_lockfile self._reference_resolver = reference_resolver self._frozen = frozen @@ -775,6 +776,12 @@ def build_dependency_tree( child is not existing_node for child in parent_node.children ): parent_node.children.append(existing_node) + if ( + self._update_keys is not None + and dep_ref.get_unique_key() in self._update_keys + and dep_ref.get_unique_key() not in self._scoped_rechecked + ): + work_items.append((existing_node, dep_ref, parent_node, is_dev)) continue # Create a new node for this dependency @@ -921,6 +928,11 @@ def build_dependency_tree( continue # Avoid infinite recursion by checking if we're already processing this dep # Use O(1) set lookup instead of O(n) list comprehension + if ( + self._update_keys is not None + and node.dependency_ref.get_unique_key() in self._update_keys + ): + self._update_keys.add(sub_dep.get_unique_key()) queue_key = sub_dep.get_resolution_key() if queue_key not in queued_keys: processing_queue.append((sub_dep, node.depth + 1, node, is_dev)) @@ -1049,14 +1061,9 @@ def _load_work_item(self, item): loop-iteration variable. """ node, dep_ref, parent_node, _is_dev = item - if self._update_keys is not None: - ancestor = parent_node - while ancestor is not None: - if ancestor.dependency_ref.get_unique_key() in self._update_keys: - with self._download_lock: - self._update_keys.add(dep_ref.get_unique_key()) - break - ancestor = ancestor.parent + if self._update_keys is not None and dep_ref.get_unique_key() in self._update_keys: + with self._download_lock: + self._scoped_rechecked.add(dep_ref.get_unique_key()) # Compute breadcrumb chain from this node's ancestry so download # errors can report "root > mid > failing-dep" context. parent_chain = node.get_ancestor_chain() diff --git a/tests/integration/test_architecture_hook_package_admission.py b/tests/integration/test_architecture_hook_package_admission.py index 740014726d..b4abdbc192 100644 --- a/tests/integration/test_architecture_hook_package_admission.py +++ b/tests/integration/test_architecture_hook_package_admission.py @@ -57,9 +57,14 @@ ), ( "src/apm_cli/deps/apm_resolver.py", - "self._update_keys.add(dep_ref.get_unique_key())", + "self._update_keys.add(sub_dep.get_unique_key())", "pass", ), + ( + "src/apm_cli/deps/apm_resolver.py", + "and dep_ref.get_unique_key() not in self._scoped_rechecked", + "and False", + ), ( "src/apm_cli/install/phases/integrate.py", "receipt[0] == install_path.resolve()", diff --git a/tests/integration/test_hook_path_package_lifecycle.py b/tests/integration/test_hook_path_package_lifecycle.py index f18edef40c..4fd39b5c16 100644 --- a/tests/integration/test_hook_path_package_lifecycle.py +++ b/tests/integration/test_hook_path_package_lifecycle.py @@ -268,13 +268,13 @@ def test_hook_path_install_restore_update_remove( @pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) @pytest.mark.parametrize("unselected_malformed", [False, True]) -@pytest.mark.parametrize("via_parent", [False, True]) +@pytest.mark.parametrize("selection_route", ["direct", "parent", "shared-child"]) def test_selective_hook_update_preserves_unselected_package( tmp_path: Path, apm_binary_path: Path, layout: str, unselected_malformed: bool, - via_parent: bool, + selection_route: str, ) -> None: isolated = IsolatedApmEnvironment.create(tmp_path / "selection", base_env=dict(os.environ)) environment = isolated.subprocess_env() @@ -309,15 +309,19 @@ def test_selective_hook_update_preserves_unselected_package( dependencies.append({"git": _REMOTE, "type": "gitlab", "path": name, "ref": "main"}) selected, unselected = map(DependencyReference.parse_from_dict, dependencies) requested = selected - if via_parent: + if selection_route != "direct": parent = repository.worktree / "parent" parent.mkdir() dump_yaml( {"name": "parent", "version": "1.0.0", "dependencies": {"apm": [dependencies[0]]}}, parent / "apm.yml", ) - dependencies[0] = {**dependencies[0], "path": "parent"} - requested = DependencyReference.parse_from_dict(dependencies[0]) + parent_dependency = {**dependencies[0], "path": "parent"} + if selection_route == "shared-child": + dependencies.insert(1, parent_dependency) + else: + dependencies[0] = parent_dependency + requested = DependencyReference.parse_from_dict(parent_dependency) repositories.commit(repository, message="seed two hook packages") repositories.install_url_rewrite(repository, _REMOTE) consumer = LocalPackageFactory(isolated.work_root).create( diff --git a/tests/unit/deps/test_hook_package_admission.py b/tests/unit/deps/test_hook_package_admission.py index 9e54a3c1f3..61d9a0335d 100644 --- a/tests/unit/deps/test_hook_package_admission.py +++ b/tests/unit/deps/test_hook_package_admission.py @@ -94,6 +94,12 @@ def test_hook_updates_reenter_staged_admission(tmp_path: Path, layout: str) -> N assert should_force_ref_recheck(ref, locked, update_refs=True, package_path=tmp_path) assert not should_force_ref_recheck(ref, locked, update_refs=False, package_path=tmp_path) + assert not should_force_ref_recheck( + ref, locked, update_refs=True, package_path=tmp_path, update_keys={"other/package"} + ) + assert should_force_ref_recheck( + ref, locked, update_refs=True, package_path=tmp_path, update_keys={ref.get_unique_key()} + ) (tmp_path / "SKILL.md").write_text("---\nname: control\n---\n# Skill\n", encoding="utf-8") assert not should_force_ref_recheck(ref, locked, update_refs=True, package_path=tmp_path) From 17f8ffb8c12ed8609cbc1a88ba1b5f7d9bf21ea1 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 19 Oct 2026 18:17:32 +0200 Subject: [PATCH 5/9] fix: preserve hook replacements during positional refresh Normalize selected dependency specs through canonical identity and reuse validated callback receipts for refresh as well as update. Cover both layouts and all receipt integrity controls. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a --- docs/src/content/docs/reference/package-types.md | 4 ++-- .../.apm/skills/apm-usage/package-authoring.md | 7 ++++--- .../checks/marketplace_catalog_and_contract.py | 7 ++++++- src/apm_cli/install/phases/integrate.py | 1 - src/apm_cli/install/phases/resolve.py | 15 ++++++++------- .../test_architecture_hook_package_admission.py | 10 ++++++++++ .../test_hook_path_package_lifecycle.py | 15 +++++++++++---- tests/unit/deps/test_hook_package_admission.py | 12 ++++++++++-- 8 files changed, 51 insertions(+), 20 deletions(-) diff --git a/docs/src/content/docs/reference/package-types.md b/docs/src/content/docs/reference/package-types.md index 36430d8b40..d36ba1e523 100644 --- a/docs/src/content/docs/reference/package-types.md +++ b/docs/src/content/docs/reference/package-types.md @@ -207,8 +207,8 @@ Installation does not execute the hooks. See for command paths, resource selection, and target restrictions. `apm install` restores the locked revision; `apm update` refreshes it. -Updating selected packages leaves unrelated installed hook packages unchanged, -even when their upstream descriptors have become invalid. +`apm update ` and `apm install --refresh` preserve unrelated +installed hook packages, despite invalid upstream descriptors. Malformed hook JSON or an invalid replacement package fails before activation, leaving the previous installation, lockfile, and deployment ownership intact. `apm uninstall` removes the package's managed hooks and resources. 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 17a4d84042..06e33624bb 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -132,11 +132,12 @@ files under `hooks/` or `.apm/hooks/`. Filename-based hook routing Hook-only Git dependencies need neither `apm.yml` nor `SKILL.md`. Set `path` to the directory containing `hooks/` or `.apm/hooks/`, not the inner hooks directory. Discovery is non-recursive. `apm install` restores -the locked revision, `apm update` refreshes it, and `apm uninstall` removes +locked revisions; `apm update` refreshes them; `apm uninstall` removes managed hooks and resources. Malformed hook descriptors or invalid replacement packages leave the prior installation, lock, and ownership -intact. Hook admission does not grant executable approval or run scripts. -Updating selected packages does not replace unrelated installed hook packages. +intact. Hook admission neither grants executable approval nor runs scripts. +`apm update ` and `apm install --refresh` preserve unrelated +installed hook packages. APM computes hook reach with one restriction-only intersection: diff --git a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py index fe675dc9ba..7acd15d656 100644 --- a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py +++ b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py @@ -742,13 +742,18 @@ def _check_package_format_precedence(provider: FactsProvider) -> tuple[Violation for path, snippets in ( ( "src/apm_cli/install/phases/resolve.py", - ("update_keys=update_keys,", "content_hash = compute_package_hash(candidate_path)"), + ( + "update_keys=update_keys,", + "content_hash = compute_package_hash(candidate_path)", + "DependencyReference.parse(p).get_unique_key()", + ), ), ( "src/apm_cli/install/phases/integrate.py", ( "receipt[0] == install_path.resolve()", "if verify_package_hash(install_path, receipt[1]):", + "already_resolved\n and bool(resolved_ref)", ), ), ): diff --git a/src/apm_cli/install/phases/integrate.py b/src/apm_cli/install/phases/integrate.py index f8460102dc..0d3513763f 100644 --- a/src/apm_cli/install/phases/integrate.py +++ b/src/apm_cli/install/phases/integrate.py @@ -230,7 +230,6 @@ def _resolve_download_strategy( _callback_sha = ctx.callback_downloaded.get(dep_key) _already_resolved_sha_match = ( already_resolved - and update_refs and bool(resolved_ref) and bool(_callback_sha) and getattr(resolved_ref, "resolved_commit", None) not in (None, "cached") diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index a1153386b2..462579229a 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -2,12 +2,8 @@ Loads inputs and populates resolution state consumed by later phases. -This is the first phase of the install pipeline. It covers: - -1. Lockfile loading (``apm.lock.yaml``) -2. ``apm_modules/`` directory creation -3. Auth resolver defaulting + downloader construction -4. Dependency resolution, ``--only`` filtering, and intended-key computation +The first install phase loads the lockfile, initializes downloads, and resolves +the selected dependency tree before integration. """ from __future__ import annotations @@ -373,6 +369,7 @@ def _resolve_dependencies( _warn_insecure_dependencies, ) from apm_cli.install.phases.local_content import _copy_local_package + from apm_cli.models.dependency.reference import DependencyReference # 3b. Dedicated registry resolver (design §3.1, §8) # Built when: @@ -424,7 +421,11 @@ def _resolve_dependencies( # lockfile entries for packages not in the manifest, unlike --update # which may restructure the whole graph). update_refs = _requires_remote_ref_resolution(ctx) - update_keys = set(ctx.only_packages) if update_refs and ctx.only_packages else None + update_keys = ( + {DependencyReference.parse(p).get_unique_key() for p in ctx.only_packages} + if update_refs and ctx.only_packages + else None + ) if ctx.refresh and ctx.logger: ctx.logger.verbose_detail("[*] --refresh: bypassing cached package content") logger = ctx.logger diff --git a/tests/integration/test_architecture_hook_package_admission.py b/tests/integration/test_architecture_hook_package_admission.py index b4abdbc192..dac78e4304 100644 --- a/tests/integration/test_architecture_hook_package_admission.py +++ b/tests/integration/test_architecture_hook_package_admission.py @@ -75,11 +75,21 @@ "if verify_package_hash(install_path, receipt[1]):", "if True:", ), + ( + "src/apm_cli/install/phases/integrate.py", + "already_resolved\n and bool(resolved_ref)", + "already_resolved\n and update_refs\n and bool(resolved_ref)", + ), ( "src/apm_cli/install/phases/resolve.py", "content_hash = compute_package_hash(candidate_path)", "content_hash = 'unverified'", ), + ( + "src/apm_cli/install/phases/resolve.py", + "DependencyReference.parse(p).get_unique_key()", + "p", + ), ], ) def test_hook_admission_cannot_bypass_canonical_owners(path: str, old: str, new: str) -> None: diff --git a/tests/integration/test_hook_path_package_lifecycle.py b/tests/integration/test_hook_path_package_lifecycle.py index 4fd39b5c16..697338cfa7 100644 --- a/tests/integration/test_hook_path_package_lifecycle.py +++ b/tests/integration/test_hook_path_package_lifecycle.py @@ -268,7 +268,7 @@ def test_hook_path_install_restore_update_remove( @pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) @pytest.mark.parametrize("unselected_malformed", [False, True]) -@pytest.mark.parametrize("selection_route", ["direct", "parent", "shared-child"]) +@pytest.mark.parametrize("selection_route", ["direct", "parent", "shared-child", "refresh"]) def test_selective_hook_update_preserves_unselected_package( tmp_path: Path, apm_binary_path: Path, @@ -278,6 +278,8 @@ def test_selective_hook_update_preserves_unselected_package( ) -> None: isolated = IsolatedApmEnvironment.create(tmp_path / "selection", base_env=dict(os.environ)) environment = isolated.subprocess_env() + if selection_route == "refresh": + environment["GITLAB_HOST"] = "gitlab.example.invalid" repositories = LocalGitRepositoryFactory(isolated.repository_root, env=environment) repository = repositories.create("hook-packages") dependencies = [] @@ -309,7 +311,7 @@ def test_selective_hook_update_preserves_unselected_package( dependencies.append({"git": _REMOTE, "type": "gitlab", "path": name, "ref": "main"}) selected, unselected = map(DependencyReference.parse_from_dict, dependencies) requested = selected - if selection_route != "direct": + if selection_route not in ("direct", "refresh"): parent = repository.worktree / "parent" parent.mkdir() dump_yaml( @@ -322,6 +324,11 @@ def test_selective_hook_update_preserves_unselected_package( else: dependencies[0] = parent_dependency requested = DependencyReference.parse_from_dict(parent_dependency) + update_args = ( + ("install", requested.to_canonical(), "--refresh", "--no-policy") + if selection_route == "refresh" + else ("update", requested.get_display_name(), "--yes") + ) + ("--parallel-downloads", "0") repositories.commit(repository, message="seed two hook packages") repositories.install_url_rewrite(repository, _REMOTE) consumer = LocalPackageFactory(isolated.work_root).create( @@ -355,7 +362,7 @@ def test_selective_hook_update_preserves_unselected_package( runner, consumer.root, environment, - ("update", requested.get_display_name(), "--yes", "--parallel-downloads", "0"), + update_args, "selection-update", ) after = LockFile.read(consumer.root / "apm.lock.yaml") @@ -378,7 +385,7 @@ def test_selective_hook_update_preserves_unselected_package( runner, consumer.root, environment, - ("update", requested.get_display_name(), "--yes", "--parallel-downloads", "0"), + update_args, "selection-invalid-update", success=False, ) diff --git a/tests/unit/deps/test_hook_package_admission.py b/tests/unit/deps/test_hook_package_admission.py index 61d9a0335d..f0b50269a4 100644 --- a/tests/unit/deps/test_hook_package_admission.py +++ b/tests/unit/deps/test_hook_package_admission.py @@ -106,7 +106,10 @@ def test_hook_updates_reenter_staged_admission(tmp_path: Path, layout: str) -> N @pytest.mark.parametrize("evidence", ["valid", "missing", "wrong-path", "tampered", "stale-sha"]) -def test_published_hook_reuse_requires_bound_integrity(tmp_path: Path, evidence: str) -> None: +@pytest.mark.parametrize("mode", ["update", "refresh"]) +def test_published_hook_reuse_requires_bound_integrity( + tmp_path: Path, evidence: str, mode: str +) -> None: ref = DependencyReference(repo_url="acme/hooks", reference="main") key = ref.get_unique_key() modules = tmp_path / "apm_modules" @@ -120,7 +123,12 @@ def test_published_hook_reuse_requires_bound_integrity(tmp_path: Path, evidence: resolved_commit="a" * 40, content_hash=compute_package_hash(installed), ) - ctx = InstallContext(project_root=tmp_path, apm_dir=tmp_path, update_refs=True) + ctx = InstallContext( + project_root=tmp_path, + apm_dir=tmp_path, + update_refs=mode == "update", + refresh=mode == "refresh", + ) ctx.apm_modules_dir = modules ctx.existing_lockfile = LockFile() ctx.existing_lockfile.add_dependency(locked) From adc9b9e493dbe592942ae1c7830bb317489da867 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 19 Oct 2026 18:39:01 +0200 Subject: [PATCH 6/9] test: bind selected hook updates to OpenAPM conformance Exercise existing req-rs-012 through installed hook lifecycles rather than waive the critical-path gate. Keep normalized selection in the existing package-selection helper and preserve invalid-selector fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a --- CONFORMANCE.json | 10 ++++++- CONFORMANCE.md | 2 +- .../marketplace_catalog_and_contract.py | 6 +++- src/apm_cli/install/package_selection.py | 13 +++++++++ src/apm_cli/install/phases/resolve.py | 12 ++------ ...est_architecture_hook_package_admission.py | 9 ++++-- .../test_hook_package_selection_reqs.py | 28 +++++++++++++++++++ 7 files changed, 66 insertions(+), 14 deletions(-) create mode 100644 tests/spec_conformance/test_hook_package_selection_reqs.py diff --git a/CONFORMANCE.json b/CONFORMANCE.json index baf8e1ab6a..35de1fa286 100644 --- a/CONFORMANCE.json +++ b/CONFORMANCE.json @@ -1155,8 +1155,16 @@ "keyword": "MUST", "section": "7.7", "status": "active", - "test_count": 1, + "test_count": 9, "tests": [ + "tests/spec_conformance/test_hook_package_selection_reqs.py::test_selected_hook_update_preserves_unrelated_pins_and_bytes[direct-.apm/hooks]", + "tests/spec_conformance/test_hook_package_selection_reqs.py::test_selected_hook_update_preserves_unrelated_pins_and_bytes[direct-hooks]", + "tests/spec_conformance/test_hook_package_selection_reqs.py::test_selected_hook_update_preserves_unrelated_pins_and_bytes[parent-.apm/hooks]", + "tests/spec_conformance/test_hook_package_selection_reqs.py::test_selected_hook_update_preserves_unrelated_pins_and_bytes[parent-hooks]", + "tests/spec_conformance/test_hook_package_selection_reqs.py::test_selected_hook_update_preserves_unrelated_pins_and_bytes[refresh-.apm/hooks]", + "tests/spec_conformance/test_hook_package_selection_reqs.py::test_selected_hook_update_preserves_unrelated_pins_and_bytes[refresh-hooks]", + "tests/spec_conformance/test_hook_package_selection_reqs.py::test_selected_hook_update_preserves_unrelated_pins_and_bytes[shared-child-.apm/hooks]", + "tests/spec_conformance/test_hook_package_selection_reqs.py::test_selected_hook_update_preserves_unrelated_pins_and_bytes[shared-child-hooks]", "tests/spec_conformance/test_resolution_reqs.py::test_resolver_records_resolved_ref_in_lockfile" ] }, diff --git a/CONFORMANCE.md b/CONFORMANCE.md index 20ea166b51..0f0969a5c7 100644 --- a/CONFORMANCE.md +++ b/CONFORMANCE.md @@ -132,7 +132,7 @@ APM claims the proposed deployed-prompt-audit capability (req-pl-019, req-pl-020 | [req-rs-009](docs/src/content/docs/specs/openapm-v0.1.md#req-rs-009) | MUST | 7.5.1 | consumer | active | 1 | - | | [req-rs-010](docs/src/content/docs/specs/openapm-v0.1.md#req-rs-010) | MUST | 7.2 | consumer | active | 3 | - | | [req-rs-011](docs/src/content/docs/specs/openapm-v0.1.md#req-rs-011) | MUST | 7.7 | consumer | active | 4 | - | -| [req-rs-012](docs/src/content/docs/specs/openapm-v0.1.md#req-rs-012) | MUST | 7.7 | consumer | active | 1 | - | +| [req-rs-012](docs/src/content/docs/specs/openapm-v0.1.md#req-rs-012) | MUST | 7.7 | consumer | active | 9 | - | | [req-rs-013](docs/src/content/docs/specs/openapm-v0.1.md#req-rs-013) | MUST | 7.2 | consumer | active | 1 | - | | [req-rs-014](docs/src/content/docs/specs/openapm-v0.1.md#req-rs-014) | MUST | 7.3.1 | consumer | active | 1 | - | | [req-rs-015](docs/src/content/docs/specs/openapm-v0.1.md#req-rs-015) | MUST | 7.5 | consumer | active | 5 | - | diff --git a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py index 7acd15d656..7a76512b30 100644 --- a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py +++ b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py @@ -745,9 +745,13 @@ def _check_package_format_precedence(provider: FactsProvider) -> tuple[Violation ( "update_keys=update_keys,", "content_hash = compute_package_hash(candidate_path)", - "DependencyReference.parse(p).get_unique_key()", + "selected_dependency_keys(ctx.only_packages)", ), ), + ( + "src/apm_cli/install/package_selection.py", + ("DependencyReference.parse(spec).get_unique_key()",), + ), ( "src/apm_cli/install/phases/integrate.py", ( diff --git a/src/apm_cli/install/package_selection.py b/src/apm_cli/install/package_selection.py index 82dec494a4..e6a17be2a9 100644 --- a/src/apm_cli/install/package_selection.py +++ b/src/apm_cli/install/package_selection.py @@ -43,3 +43,16 @@ def only_packages_from_validation( seen.add(canonical) selected.append(canonical) return selected + + +def selected_dependency_keys(packages: list[str]) -> set[str]: + """Normalize selected specs while retaining the existing raw-selector fallback.""" + from apm_cli.models.dependency.reference import DependencyReference + + keys: set[str] = set() + for spec in packages: + try: + keys.add(DependencyReference.parse(spec).get_unique_key()) + except (ValueError, TypeError): + keys.add(spec) + return keys diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index 462579229a..2478399e7b 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -1,8 +1,4 @@ -"""Dependency resolution phase. - -Loads inputs and populates resolution state consumed by later phases. - -The first install phase loads the lockfile, initializes downloads, and resolves +"""Load the lockfile, initialize downloads, and resolve the selected dependency tree before integration. """ @@ -368,8 +364,8 @@ def _resolve_dependencies( _guard_transitive_insecure_dependencies, _warn_insecure_dependencies, ) + from apm_cli.install.package_selection import selected_dependency_keys from apm_cli.install.phases.local_content import _copy_local_package - from apm_cli.models.dependency.reference import DependencyReference # 3b. Dedicated registry resolver (design §3.1, §8) # Built when: @@ -422,9 +418,7 @@ def _resolve_dependencies( # which may restructure the whole graph). update_refs = _requires_remote_ref_resolution(ctx) update_keys = ( - {DependencyReference.parse(p).get_unique_key() for p in ctx.only_packages} - if update_refs and ctx.only_packages - else None + selected_dependency_keys(ctx.only_packages) if update_refs and ctx.only_packages else None ) if ctx.refresh and ctx.logger: ctx.logger.verbose_detail("[*] --refresh: bypassing cached package content") diff --git a/tests/integration/test_architecture_hook_package_admission.py b/tests/integration/test_architecture_hook_package_admission.py index dac78e4304..2f7bd1748b 100644 --- a/tests/integration/test_architecture_hook_package_admission.py +++ b/tests/integration/test_architecture_hook_package_admission.py @@ -87,8 +87,13 @@ ), ( "src/apm_cli/install/phases/resolve.py", - "DependencyReference.parse(p).get_unique_key()", - "p", + "selected_dependency_keys(ctx.only_packages)", + "set(ctx.only_packages)", + ), + ( + "src/apm_cli/install/package_selection.py", + "DependencyReference.parse(spec).get_unique_key()", + "spec", ), ], ) diff --git a/tests/spec_conformance/test_hook_package_selection_reqs.py b/tests/spec_conformance/test_hook_package_selection_reqs.py new file mode 100644 index 0000000000..968129d1ca --- /dev/null +++ b/tests/spec_conformance/test_hook_package_selection_reqs.py @@ -0,0 +1,28 @@ +"""Bind selective hook updates to the existing subtree and prior-pin contract.""" + +from pathlib import Path + +import pytest + +from tests.integration.conftest import apm_binary_path as apm_binary_path +from tests.integration.test_hook_path_package_lifecycle import ( + test_selective_hook_update_preserves_unselected_package as _run_selected_hook_contract, +) + +pytestmark = [pytest.mark.e2e, pytest.mark.requires_apm_binary] + + +@pytest.mark.req("req-rs-012") +@pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) +@pytest.mark.parametrize("selection_route", ["direct", "parent", "shared-child", "refresh"]) +def test_selected_hook_update_preserves_unrelated_pins_and_bytes( + tmp_path: Path, apm_binary_path: Path, layout: str, selection_route: str +) -> None: + """Exercise real installs, subtree updates, and malformed unselected sources.""" + _run_selected_hook_contract( + tmp_path, + apm_binary_path, + layout, + unselected_malformed=True, + selection_route=selection_route, + ) From df4e420f2b18c21e784f31586680c1072b1238ec Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 19 Oct 2026 18:56:15 +0200 Subject: [PATCH 7/9] test: cover selected-key fallback directly Name the canonical delegate and verify both parse failure types without losing valid selected keys. No runtime behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a --- src/apm_cli/install/package_selection.py | 2 +- tests/unit/install/test_package_selection.py | 26 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 tests/unit/install/test_package_selection.py diff --git a/src/apm_cli/install/package_selection.py b/src/apm_cli/install/package_selection.py index e6a17be2a9..86ddcb2431 100644 --- a/src/apm_cli/install/package_selection.py +++ b/src/apm_cli/install/package_selection.py @@ -46,7 +46,7 @@ def only_packages_from_validation( def selected_dependency_keys(packages: list[str]) -> set[str]: - """Normalize selected specs while retaining the existing raw-selector fallback.""" + """Delegate to DependencyReference.parse/get_unique_key, retaining raw selectors on failure.""" from apm_cli.models.dependency.reference import DependencyReference keys: set[str] = set() diff --git a/tests/unit/install/test_package_selection.py b/tests/unit/install/test_package_selection.py new file mode 100644 index 0000000000..5d6d30e046 --- /dev/null +++ b/tests/unit/install/test_package_selection.py @@ -0,0 +1,26 @@ +"""Pure selected-package identity normalization and raw-selector fallback.""" + +import pytest + +from apm_cli.install.package_selection import selected_dependency_keys +from apm_cli.models.dependency.reference import DependencyReference + +pytestmark = pytest.mark.unit + + +@pytest.mark.parametrize("error", [ValueError, TypeError]) +def test_selected_keys_preserve_failed_selector_without_losing_valid_keys( + monkeypatch: pytest.MonkeyPatch, error: type[Exception] +) -> None: + original_parse = DependencyReference.parse + + def parse(spec: str) -> DependencyReference: + if spec == "INVALID_REF_NO_SLASH": + raise error("invalid selector") + return original_parse(spec) + + monkeypatch.setattr(DependencyReference, "parse", parse) + assert selected_dependency_keys(["org/pkg#main", "INVALID_REF_NO_SLASH"]) == { + "org/pkg", + "INVALID_REF_NO_SLASH", + } From deffc8e4d361f419b8fd972cdff8289c8f97cfc3 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 19 Oct 2026 19:41:43 +0200 Subject: [PATCH 8/9] fix: retain locked integrity for ordinary installs Restrict validated callback receipts to update and refresh modes. Preserve ordinary and frozen lock restoration, with explicit negative mode coverage and matching architecture guard mutations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a --- .../checks/marketplace_catalog_and_contract.py | 2 +- src/apm_cli/install/phases/integrate.py | 3 ++- .../test_architecture_hook_package_admission.py | 9 +++++++-- tests/unit/deps/test_hook_package_admission.py | 5 +++-- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py index 7a76512b30..58717c8c61 100644 --- a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py +++ b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py @@ -757,7 +757,7 @@ def _check_package_format_precedence(provider: FactsProvider) -> tuple[Violation ( "receipt[0] == install_path.resolve()", "if verify_package_hash(install_path, receipt[1]):", - "already_resolved\n and bool(resolved_ref)", + "and (update_refs or ctx.refresh)", ), ), ): diff --git a/src/apm_cli/install/phases/integrate.py b/src/apm_cli/install/phases/integrate.py index 0d3513763f..3b40510944 100644 --- a/src/apm_cli/install/phases/integrate.py +++ b/src/apm_cli/install/phases/integrate.py @@ -223,13 +223,14 @@ def _resolve_download_strategy( # Issue #551: skip re-download when the BFS callback already fetched this # dep during resolution AND the remote SHA still matches what was captured. - # This eliminates redundant network I/O in --update mode when apm_modules/ + # This eliminates redundant network I/O in update/refresh when apm_modules/ # is empty but the lockfile SHA is stale: the callback downloads the latest # content (recording its SHA), and the sequential loop would otherwise # re-download identical bytes because lockfile_match=False (stale SHA). _callback_sha = ctx.callback_downloaded.get(dep_key) _already_resolved_sha_match = ( already_resolved + and (update_refs or ctx.refresh) and bool(resolved_ref) and bool(_callback_sha) and getattr(resolved_ref, "resolved_commit", None) not in (None, "cached") diff --git a/tests/integration/test_architecture_hook_package_admission.py b/tests/integration/test_architecture_hook_package_admission.py index 2f7bd1748b..01a3ef0488 100644 --- a/tests/integration/test_architecture_hook_package_admission.py +++ b/tests/integration/test_architecture_hook_package_admission.py @@ -77,8 +77,13 @@ ), ( "src/apm_cli/install/phases/integrate.py", - "already_resolved\n and bool(resolved_ref)", - "already_resolved\n and update_refs\n and bool(resolved_ref)", + "and (update_refs or ctx.refresh)", + "and update_refs", + ), + ( + "src/apm_cli/install/phases/integrate.py", + "and (update_refs or ctx.refresh)", + "and True", ), ( "src/apm_cli/install/phases/resolve.py", diff --git a/tests/unit/deps/test_hook_package_admission.py b/tests/unit/deps/test_hook_package_admission.py index f0b50269a4..e04c0a27ad 100644 --- a/tests/unit/deps/test_hook_package_admission.py +++ b/tests/unit/deps/test_hook_package_admission.py @@ -106,7 +106,7 @@ def test_hook_updates_reenter_staged_admission(tmp_path: Path, layout: str) -> N @pytest.mark.parametrize("evidence", ["valid", "missing", "wrong-path", "tampered", "stale-sha"]) -@pytest.mark.parametrize("mode", ["update", "refresh"]) +@pytest.mark.parametrize("mode", ["update", "refresh", "install"]) def test_published_hook_reuse_requires_bound_integrity( tmp_path: Path, evidence: str, mode: str ) -> None: @@ -169,7 +169,8 @@ def test_published_hook_reuse_requires_bound_integrity( resolved, skip, locked_check, changed = _resolve_download_strategy(ctx, ref, installed) - assert skip is (evidence == "valid") + # Ordinary installs check the locked hash, not the fresh callback receipt. + assert skip is (evidence == "valid" and mode != "install") if skip: source = make_dependency_source( ctx, From 04e7a0fb8600eb455b5bf76c3e2de0643fdb2226 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 19 Oct 2026 23:04:11 +0200 Subject: [PATCH 9/9] fix(install): share interpreted selector identity before staged rechecks Repair the human-authorized 3156/3282 versioned marketplace hook refresh interaction without changing parser, staging, receipt or trust semantics. Preserve installed source and full lifecycle state on rejected replacements. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a --- .../content/docs/reference/package-types.md | 2 + .../skills/apm-usage/package-authoring.md | 3 + .../checks/install_dry_run_plan.py | 23 +++- .../marketplace_catalog_and_contract.py | 4 +- src/apm_cli/install/package_selection.py | 25 ++-- src/apm_cli/install/phases/resolve.py | 4 +- ...est_architecture_hook_package_admission.py | 4 +- .../test_architecture_owner_rule_mutations.py | 20 ++++ .../test_hook_path_package_lifecycle.py | 110 ++++++++++++++++++ tests/unit/install/test_package_selection.py | 15 ++- 10 files changed, 194 insertions(+), 16 deletions(-) diff --git a/docs/src/content/docs/reference/package-types.md b/docs/src/content/docs/reference/package-types.md index d36ba1e523..6a7726ba3a 100644 --- a/docs/src/content/docs/reference/package-types.md +++ b/docs/src/content/docs/reference/package-types.md @@ -211,6 +211,8 @@ for command paths, resource selection, and target restrictions. installed hook packages, despite invalid upstream descriptors. Malformed hook JSON or an invalid replacement package fails before activation, leaving the previous installation, lockfile, and deployment ownership intact. +This also applies to `apm install plugin@marketplace --refresh` when a catalog +selects a versioned hook-package directory, such as `plugins/hooks-1.2.3`. `apm uninstall` removes the package's managed hooks and resources. **When to choose:** you ship a set of harness hooks with no other 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 06e33624bb..61aa471728 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -138,6 +138,9 @@ replacement packages leave the prior installation, lock, and ownership intact. Hook admission neither grants executable approval nor runs scripts. `apm update ` and `apm install --refresh` preserve unrelated installed hook packages. +For marketplace-selected versioned hook directories, use +`apm install plugin@marketplace --refresh`; a rejected replacement also preserves +the installed source bytes, lock, and deployment ownership. APM computes hook reach with one restriction-only intersection: diff --git a/scripts/architecture_linter/checks/install_dry_run_plan.py b/scripts/architecture_linter/checks/install_dry_run_plan.py index f9d531f72f..9955d1475d 100644 --- a/scripts/architecture_linter/checks/install_dry_run_plan.py +++ b/scripts/architecture_linter/checks/install_dry_run_plan.py @@ -58,7 +58,14 @@ def check_prospective_dry_run_plan(provider: FactsProvider) -> tuple[Violation, command, command_fail = _facts_for(provider, _COMMAND, rule_id) renderer, renderer_fail = _facts_for(provider, _RENDERER, rule_id) selection, selection_fail = _facts_for(provider, _SELECTION_OWNER, rule_id) - failures = list(owner_fail) + list(command_fail) + list(renderer_fail) + list(selection_fail) + resolve, resolve_fail = _facts_for(provider, _RESOLVE, rule_id) + failures = ( + list(owner_fail) + + list(command_fail) + + list(renderer_fail) + + list(selection_fail) + + list(resolve_fail) + ) if failures: return tuple(failures) @@ -86,6 +93,20 @@ def check_prospective_dry_run_plan(provider: FactsProvider) -> tuple[Violation, _count_re(owner, class_pattern) == 1 and _count_re(selection, identity_pattern) == 1 and _selection_uses_interpreted_identity(provider) + and _present(selection, "for dependency in dependencies:") + and _present(selection, "if dependency.to_canonical() == package:") + and _present( + selection, + "return _selected_dependency_reference(package, dependencies).get_identity()", + ) + and _present( + selection, + "_selected_dependency_reference(spec, dependencies).get_unique_key()", + ) + and _present( + resolve, + "selected_dependency_keys(ctx.only_packages, ctx.all_apm_deps)", + ) and _present(owner, "def from_apm_package(") and _present(owner, "def with_allowed_lsp_dependencies(") and _present(owner, "selected_apm_dependencies=selected_apm_dependencies") diff --git a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py index 58717c8c61..aa2faa9809 100644 --- a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py +++ b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py @@ -745,12 +745,12 @@ def _check_package_format_precedence(provider: FactsProvider) -> tuple[Violation ( "update_keys=update_keys,", "content_hash = compute_package_hash(candidate_path)", - "selected_dependency_keys(ctx.only_packages)", + "selected_dependency_keys(ctx.only_packages, ctx.all_apm_deps)", ), ), ( "src/apm_cli/install/package_selection.py", - ("DependencyReference.parse(spec).get_unique_key()",), + ("_selected_dependency_reference(spec, dependencies).get_unique_key()",), ), ( "src/apm_cli/install/phases/integrate.py", diff --git a/src/apm_cli/install/package_selection.py b/src/apm_cli/install/package_selection.py index c79b9555f0..c3f822c5ab 100644 --- a/src/apm_cli/install/package_selection.py +++ b/src/apm_cli/install/package_selection.py @@ -11,14 +11,21 @@ from apm_cli.core.command_logger import _ValidationOutcome -def selected_dependency_identity(package: str, dependencies: Sequence[DependencyReference]) -> str: - """Resolve a selector using interpreted references before shorthand parsing.""" +def _selected_dependency_reference( + package: str, dependencies: Sequence[DependencyReference] +) -> DependencyReference: + """Preserve interpreted coordinates before falling back to shorthand parsing.""" for dependency in dependencies: # Explicit git+path coordinates can contain dotted bundle directories # whose canonical display is not a valid shorthand file reference. if dependency.to_canonical() == package: - return dependency.get_identity() - return DependencyReference.parse(package).get_identity() + return dependency + return DependencyReference.parse(package) + + +def selected_dependency_identity(package: str, dependencies: Sequence[DependencyReference]) -> str: + """Resolve a selector using interpreted references before shorthand parsing.""" + return _selected_dependency_reference(package, dependencies).get_identity() def existing_dependency_identities(current_dependencies: list[object]) -> set[str]: @@ -56,14 +63,14 @@ def only_packages_from_validation( return selected -def selected_dependency_keys(packages: list[str]) -> set[str]: - """Delegate to DependencyReference.parse/get_unique_key, retaining raw selectors on failure.""" - from apm_cli.models.dependency.reference import DependencyReference - +def selected_dependency_keys( + packages: list[str], dependencies: Sequence[DependencyReference] = () +) -> set[str]: + """Use the shared interpreted selector owner, retaining raw selectors on failure.""" keys: set[str] = set() for spec in packages: try: - keys.add(DependencyReference.parse(spec).get_unique_key()) + keys.add(_selected_dependency_reference(spec, dependencies).get_unique_key()) except (ValueError, TypeError): keys.add(spec) return keys diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index ff16de22e9..260bbc8133 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -418,7 +418,9 @@ def _resolve_dependencies( # which may restructure the whole graph). update_refs = _requires_remote_ref_resolution(ctx) update_keys = ( - selected_dependency_keys(ctx.only_packages) if update_refs and ctx.only_packages else None + selected_dependency_keys(ctx.only_packages, ctx.all_apm_deps) + if update_refs and ctx.only_packages + else None ) if ctx.refresh and ctx.logger: ctx.logger.verbose_detail("[*] --refresh: bypassing cached package content") diff --git a/tests/integration/test_architecture_hook_package_admission.py b/tests/integration/test_architecture_hook_package_admission.py index 01a3ef0488..7f94245f1b 100644 --- a/tests/integration/test_architecture_hook_package_admission.py +++ b/tests/integration/test_architecture_hook_package_admission.py @@ -92,12 +92,12 @@ ), ( "src/apm_cli/install/phases/resolve.py", - "selected_dependency_keys(ctx.only_packages)", + "selected_dependency_keys(ctx.only_packages, ctx.all_apm_deps)", "set(ctx.only_packages)", ), ( "src/apm_cli/install/package_selection.py", - "DependencyReference.parse(spec).get_unique_key()", + "_selected_dependency_reference(spec, dependencies).get_unique_key()", "spec", ), ], diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index 6b6ef864be..0607b664c9 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -68,6 +68,26 @@ "selected_dependency_identity(package, all_apm_dependencies)", "DependencyReference.parse(package).get_identity()", ), + ( + "src/apm_cli/install/phases/resolve.py", + "selected_dependency_keys(ctx.only_packages, ctx.all_apm_deps)", + "selected_dependency_keys(ctx.only_packages)", + ), + ( + "src/apm_cli/install/package_selection.py", + "for dependency in dependencies:", + "for dependency in ():", + ), + ( + "src/apm_cli/install/package_selection.py", + "_selected_dependency_reference(spec, dependencies).get_unique_key()", + "DependencyReference.parse(spec).get_unique_key()", + ), + ( + "src/apm_cli/install/package_selection.py", + "return _selected_dependency_reference(package, dependencies).get_identity()", + "return DependencyReference.parse(package).get_identity()", + ), ], ) def test_package_selector_identity_cannot_bypass_interpreted_references( diff --git a/tests/integration/test_hook_path_package_lifecycle.py b/tests/integration/test_hook_path_package_lifecycle.py index 697338cfa7..804d100306 100644 --- a/tests/integration/test_hook_path_package_lifecycle.py +++ b/tests/integration/test_hook_path_package_lifecycle.py @@ -54,6 +54,116 @@ def _source_bytes(installed: Path) -> dict[str, bytes]: } +@pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) +def test_versioned_marketplace_hook_refresh_preserves_invalid_replacement( + tmp_path: Path, apm_engine_command: tuple[str, ...], layout: str +) -> None: + """Interpreted versioned selectors must stage replacements before publication.""" + isolated = IsolatedApmEnvironment.create(tmp_path / "scenario", base_env=dict(os.environ)) + repositories = LocalGitRepositoryFactory( + isolated.repository_root, env=isolated.subprocess_env() + ) + repository = repositories.create("versioned-hook-marketplace") + remote = "https://github.com/acme/versioned-hook-marketplace" + virtual_path = "plugins/hook-bundle-1.2.3" + descriptor = repository.worktree / virtual_path / layout / "lint.json" + descriptor.parent.mkdir(parents=True) + descriptor.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "hooks": [ + { + "type": "command", + "command": f'python "${{CLAUDE_PLUGIN_ROOT}}/{layout}/run.py" v1', + } + ] + } + ] + } + } + ), + encoding="utf-8", + ) + (descriptor.parent / "run.py").write_text( + 'from pathlib import Path\nPath("hook-ran.txt").write_text("unexpected")\n', + encoding="utf-8", + ) + (descriptor.parent / "resource.txt").write_text("hook resource", encoding="utf-8") + catalog = repository.worktree / ".claude-plugin/marketplace.json" + catalog.parent.mkdir() + catalog.write_text( + json.dumps( + { + "name": "hook-catalog", + "owner": {"name": "Fixture"}, + "plugins": [ + { + "name": "hook-bundle", + "source": { + "source": "github", + "repo": "acme/versioned-hook-marketplace", + "path": virtual_path, + "ref": "main", + }, + } + ], + } + ), + encoding="utf-8", + ) + repositories.commit(repository, message="seed versioned hook marketplace") + environment = repositories.url_rewrite_subprocess_env(repository, remote) + consumer = LocalPackageFactory(isolated.work_root).create("consumer", targets=("claude",)) + # Only the catalog transport changes; package validation and acquisition remain real. + runner = ApmLifecycleRunner( + ( + apm_engine_command[0], + "-c", + "from apm_cli.marketplace import client\n" + "client._FETCHERS['github'] = client._fetch_git\n" + "from apm_cli.cli import cli\ncli()\n", + ) + ) + _run( + runner, + consumer.root, + environment, + ("marketplace", "add", remote, "--name", "hook-catalog", "--ref", "main"), + "versioned-hook-register", + ) + install = ("install", "hook-bundle@hook-catalog", "--target", "claude", "--no-policy") + _run(runner, consumer.root, environment, install, "versioned-hook-install") + ref = DependencyReference.parse_from_dict({"git": remote, "path": virtual_path, "ref": "main"}) + installed = ref.get_install_path(consumer.root / "apm_modules") + assert (installed / layout / "lint.json").is_file() + descriptor.write_text( + descriptor.read_text(encoding="utf-8").replace("v1", "v2"), encoding="utf-8" + ) + repositories.commit(repository, message="advance versioned hook") + _run(runner, consumer.root, environment, (*install, "--refresh"), "versioned-hook-refresh") + assert "v2" in (installed / layout / "lint.json").read_text(encoding="utf-8") + before_source = _source_bytes(installed) + before_state = _snapshot(consumer.root) + assert before_state.deployment_records + assert any("hook-sidecar" in state.roles and state.content for state in before_state.files) + descriptor.write_text("{", encoding="utf-8") + repositories.commit(repository, message="invalidate versioned hook") + _run( + runner, + consumer.root, + environment, + (*install, "--refresh"), + "versioned-hook-reject-invalid", + success=False, + ) + assert _snapshot(consumer.root) == before_state + assert _source_bytes(installed) == before_source + assert not (consumer.root / "hook-ran.txt").exists() + + def _assert_hooks( project: Path, marker: str, configs: tuple[PurePosixPath, ...] = _CONFIGS ) -> None: diff --git a/tests/unit/install/test_package_selection.py b/tests/unit/install/test_package_selection.py index 5d6d30e046..d5dc08ab27 100644 --- a/tests/unit/install/test_package_selection.py +++ b/tests/unit/install/test_package_selection.py @@ -2,7 +2,7 @@ import pytest -from apm_cli.install.package_selection import selected_dependency_keys +from apm_cli.install.package_selection import selected_dependency_identity, selected_dependency_keys from apm_cli.models.dependency.reference import DependencyReference pytestmark = pytest.mark.unit @@ -24,3 +24,16 @@ def parse(spec: str) -> DependencyReference: "org/pkg", "INVALID_REF_NO_SLASH", } + + +@pytest.mark.parametrize("ref", ["main", "release/hooks", "v1.2.3"]) +def test_selected_keys_and_identity_preserve_interpreted_versioned_paths(ref: str) -> None: + dependency = DependencyReference.parse_from_dict( + {"git": "https://github.com/acme/hooks", "path": "plugins/hook-1.2.3", "ref": ref} + ) + selector = dependency.to_canonical() + assert selected_dependency_keys([selector, "other/package#main"], [dependency]) == { + dependency.get_unique_key(), + "other/package", + } + assert selected_dependency_identity(selector, [dependency]) == dependency.get_identity()