diff --git a/CHANGELOG.md b/CHANGELOG.md index 616c518d51..51e1e866ae 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) - Codex MCP entries no longer emit the unsupported `id` setting; reinstalling repairs recorded APM-owned entries while preserving registry identity, user settings, and ownership safeguards. -- by @Pybsama (#3157) - Git marketplace installs accept versioned plugin directories while rejecting file, symlink, and submodule responses during directory validation. -- by @Pybsama (#3156) - `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) diff --git a/CONFORMANCE.json b/CONFORMANCE.json index 5c03df4a12..752c6b99c0 100644 --- a/CONFORMANCE.json +++ b/CONFORMANCE.json @@ -1161,8 +1161,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 72fc9b0fab..7233262505 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/docs/src/content/docs/reference/package-types.md b/docs/src/content/docs/reference/package-types.md index d81d11d1ce..6a7726ba3a 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,40 @@ 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. +`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. +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 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 35f0b9857a..61aa471728 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 | 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 | @@ -129,6 +129,19 @@ 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 +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 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: `effective hook targets = project active targets INTERSECT consumer 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 f083749da2..aa2faa9809 100644 --- a/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py +++ b/scripts/architecture_linter/checks/marketplace_catalog_and_contract.py @@ -694,6 +694,94 @@ 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(", + "update_keys=self._update_keys,", + "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", + ) + ) + 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:", + "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)", + "selected_dependency_keys(ctx.only_packages, ctx.all_apm_deps)", + ), + ), + ( + "src/apm_cli/install/package_selection.py", + ("_selected_dependency_reference(spec, dependencies).get_unique_key()",), + ), + ( + "src/apm_cli/install/phases/integrate.py", + ( + "receipt[0] == install_path.resolve()", + "if verify_package_hash(install_path, receipt[1]):", + "and (update_refs or ctx.refresh)", + ), + ), + ): + 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, + 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..cdb841435e 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,8 @@ 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._scoped_rechecked: set[str] = set() self._existing_lockfile = existing_lockfile self._reference_resolver = reference_resolver self._frozen = frozen @@ -771,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 @@ -917,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)) @@ -1045,6 +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 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() @@ -1085,6 +1104,12 @@ 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 + else None + ), ) def _try_load_dependency_package( @@ -1265,7 +1290,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 +1310,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..da73635c2a 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,20 +202,32 @@ def should_force_ref_recheck( locked_dep: LockedDependency | None, *, 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. 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: - return getattr(dep_ref, "ref_kind", None) == "semver" + 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 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/package_selection.py b/src/apm_cli/install/package_selection.py index 2b9bb22f3c..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]: @@ -54,3 +61,16 @@ def only_packages_from_validation( seen.add(canonical) selected.append(canonical) return selected + + +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(_selected_dependency_reference(spec, dependencies).get_unique_key()) + except (ValueError, TypeError): + keys.add(spec) + return keys diff --git a/src/apm_cli/install/phases/integrate.py b/src/apm_cli/install/phases/integrate.py index 295b326a37..3b40510944 100644 --- a/src/apm_cli/install/phases/integrate.py +++ b/src/apm_cli/install/phases/integrate.py @@ -223,14 +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 + 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") @@ -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 4de8ede143..260bbc8133 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -1,13 +1,5 @@ -"""Dependency resolution phase. - -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 +"""Load the lockfile, initialize downloads, and resolve +the selected dependency tree before integration. """ from __future__ import annotations @@ -155,8 +147,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 +160,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 @@ -366,6 +364,7 @@ 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 # 3b. Dedicated registry resolver (design §3.1, §8) @@ -418,6 +417,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 = ( + 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") logger = ctx.logger @@ -467,6 +471,8 @@ 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, + update_keys=update_keys, ): return install_path replacement_path = staging_session.prepare_replacement(install_path) @@ -745,9 +751,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, @@ -758,18 +766,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) @@ -898,9 +895,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/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/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 new file mode 100644 index 0000000000..7f94245f1b --- /dev/null +++ b/tests/integration/test_architecture_hook_package_admission.py @@ -0,0 +1,112 @@ +"""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", + ), + ( + "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(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()", + "True", + ), + ( + "src/apm_cli/install/phases/integrate.py", + "if verify_package_hash(install_path, receipt[1]):", + "if True:", + ), + ( + "src/apm_cli/install/phases/integrate.py", + "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", + "content_hash = compute_package_hash(candidate_path)", + "content_hash = 'unverified'", + ), + ( + "src/apm_cli/install/phases/resolve.py", + "selected_dependency_keys(ctx.only_packages, ctx.all_apm_deps)", + "set(ctx.only_packages)", + ), + ( + "src/apm_cli/install/package_selection.py", + "_selected_dependency_reference(spec, dependencies).get_unique_key()", + "spec", + ), + ], +) +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_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index 0f9a07d01a..3f85270436 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 new file mode 100644 index 0000000000..804d100306 --- /dev/null +++ b/tests/integration/test_hook_path_package_lifecycle.py @@ -0,0 +1,504 @@ +"""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 _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() + } + + +@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: + 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") + 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") + + 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 = _source_bytes(installed) + 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 _source_bytes(installed) == 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() + + +@pytest.mark.parametrize("layout", ["hooks", ".apm/hooks"]) +@pytest.mark.parametrize("unselected_malformed", [False, True]) +@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, + layout: str, + unselected_malformed: bool, + selection_route: str, +) -> 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 = [] + 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 selection_route not in ("direct", "refresh"): + parent = repository.worktree / "parent" + parent.mkdir() + dump_yaml( + {"name": "parent", "version": "1.0.0", "dependencies": {"apm": [dependencies[0]]}}, + parent / "apm.yml", + ) + 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) + 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( + "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_args, + "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_args, + "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/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, + ) 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..e04c0a27ad --- /dev/null +++ b/tests/unit/deps/test_hook_package_admission.py @@ -0,0 +1,193 @@ +"""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, 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 + +_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) + 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) + + +@pytest.mark.parametrize("evidence", ["valid", "missing", "wrong-path", "tampered", "stale-sha"]) +@pytest.mark.parametrize("mode", ["update", "refresh", "install"]) +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" + 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=mode == "update", + refresh=mode == "refresh", + ) + 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) + + # 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, + 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/install/test_package_selection.py b/tests/unit/install/test_package_selection.py new file mode 100644 index 0000000000..d5dc08ab27 --- /dev/null +++ b/tests/unit/install/test_package_selection.py @@ -0,0 +1,39 @@ +"""Pure selected-package identity normalization and raw-selector fallback.""" + +import pytest + +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 + + +@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", + } + + +@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() 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"