From cecf16bec77fd4b49e3786210ec3b1e3d7409b9f Mon Sep 17 00:00:00 2001 From: parth Date: Fri, 2 Oct 2026 22:42:20 +0530 Subject: [PATCH 1/4] fix(hooks): type untyped flat command handlers for Claude Flat hook command entries that omit `type` (valid in Cursor, where it defaults to `command`) were wrapped into Claude's matcher groups without the handler `type` field that Claude's hook schema requires. The Claude renderer now supplies `"type": "command"` for flat entries that the neutral hook grammar reads as command handlers. Explicit handler types, untyped handlers already inside nested Claude groups, non-command entries, and all other targets are unchanged. Reinstall cleanup also matches the pre-fix untyped render, so stale root-package entries keep healing instead of duplicating (the #1329 / #1392 contract), mirroring the existing Codex legacy-content-key path. Refs #3130 apm-spec-waiver: Claude-native handler type default for #3130; OpenAPM v0.1 defines no target-native hook handler fields, the portable hook subset is deferred to #2111 (out of approved scope), and req-lk-021 ownership reconciliation is preserved. --- CHANGELOG.md | 4 + .../author-primitives/hooks-and-commands.md | 7 +- .../skills/apm-usage/package-authoring.md | 4 + src/apm_cli/integration/hook_integrator.py | 6 + .../integration/hook_native_formats.py | 25 +- .../test_hook_integrator_issue3130.py | 261 ++++++++++++++++++ 6 files changed, 303 insertions(+), 4 deletions(-) create mode 100644 tests/unit/integration/test_hook_integrator_issue3130.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c1a995f25..d954b38b10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `apm install` now writes flat hook command entries that omit `type` to `.claude/settings.json` as schema-valid `"type": "command"` handlers, including when merged next to a Claude-shaped hook file. Explicit handler types and other targets are unchanged; reinstall to refresh existing entries. (by @Parth-Vasave, #3130) + ## [0.33.0] - 2026-10-02 ### Added diff --git a/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md b/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md index 7265b8167d..691a986923 100644 --- a/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md +++ b/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md @@ -196,8 +196,11 @@ Supported targets and where the integrator writes: APM parses the source into vendor-neutral hook intent, then each target integrator renders its native schema. Flat command entries become Claude's required `{ "matcher": "*", "hooks": [...] }` entries in -`.claude/settings.json`. For Codex, flat command entries become intermediate -hook groups containing a nested `hooks` array in `.codex/hooks.json`. Kiro +`.claude/settings.json`; a flat command entry that omits `type` receives +Claude's required `"type": "command"`, while explicit handler types are kept +and other targets receive no default. For Codex, flat command entries become +intermediate hook groups containing a nested `hooks` array in +`.codex/hooks.json`. Kiro receives its current v1 standalone schema: `{ "version": "v1", "hooks": [{ "name", "trigger", "matcher", "action" }] }`. Kiro trigger names are PascalCase and command timeouts remain in seconds. 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 ba6026039f..af9a68cd23 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -181,6 +181,10 @@ becomes `PostToolUse` in Claude) and rewrites path variables the correct target-specific form. Kiro materializes one JSON document per hook action under `.kiro/hooks/`. +For Claude, APM wraps flat command entries in `{ "matcher", "hooks": [...] }` +groups in `.claude/settings.json` and adds the required `"type": "command"` +when a flat command entry omits `type`. Explicit handler types are kept. + For Codex, APM wraps flat command entries in hook groups containing a nested `hooks` array in `.codex/hooks.json`. diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index c1606db175..e0b0c63a53 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -1353,6 +1353,12 @@ def _integrate_merged_hooks( json_config[container][event_name] = [] legacy_content_keys: set[str] = set() if config.target_key == "claude": + # Match owned untyped groups from installs before handler typing. + legacy_content_keys = { + self._hook_entry_content_key(entry) + for entry in _to_claude_hook_entries(entries, default_handler_type=False) + if isinstance(entry, dict) + } entries = _to_claude_hook_entries(entries) elif config.target_key == "codex": # Match owned flat entries from installs before Codex nesting. diff --git a/src/apm_cli/integration/hook_native_formats.py b/src/apm_cli/integration/hook_native_formats.py index 994c12fa7c..84b4f0d3e7 100644 --- a/src/apm_cli/integration/hook_native_formats.py +++ b/src/apm_cli/integration/hook_native_formats.py @@ -221,8 +221,29 @@ def _to_gemini_hook_entries(entries: list) -> list: ) -def _to_claude_hook_entries(entries: list) -> list: - """Render portable bindings in Claude's nested matcher schema.""" +def _with_claude_default_handler_type(entry: object) -> object: + """Supply Claude's required ``type`` for an untyped flat command entry. + + Only flat entries that the neutral grammar reads as a command handler are + defaulted; explicit types, nested groups, and non-command entries pass + through unchanged. + """ + if not isinstance(entry, dict) or "type" in entry or isinstance(entry.get("hooks"), list): + return entry + if not isinstance(_handler_to_ir(entry, None).command, str): + return entry + return {"type": "command", **entry} + + +def _to_claude_hook_entries(entries: list, *, default_handler_type: bool = True) -> list: + """Render portable bindings in Claude's nested matcher schema. + + ``default_handler_type=False`` reproduces the output of installs before + untyped flat command entries were typed, so owned entries they wrote can + still be matched on reinstall. + """ + if default_handler_type: + entries = [_with_claude_default_handler_type(entry) for entry in entries] return _render_nested_document( _entries_to_ir(entries), timeout_milliseconds=False, diff --git a/tests/unit/integration/test_hook_integrator_issue3130.py b/tests/unit/integration/test_hook_integrator_issue3130.py new file mode 100644 index 0000000000..5795d85964 --- /dev/null +++ b/tests/unit/integration/test_hook_integrator_issue3130.py @@ -0,0 +1,261 @@ +"""Regression tests for #3130 -- untyped flat hook handlers in Claude settings. + +A flat hook entry may omit ``type`` (valid in Cursor, where it defaults to +``command``). The Claude render wrapped such entries into Claude's +``{"matcher", "hooks": [...]}`` groups but left the handler without the +``type`` field that Claude's hook schema requires. + +These tests assert the post-fix contract: +- Untyped flat command entries render as ``"type": "command"`` handlers in + ``.claude/settings.json`` and its ownership sidecar, both standalone and + when merged next to a Claude-shaped hook file from the same package. +- Explicit handler types and untyped entries that are not command handlers + are preserved unchanged. +- Untyped handlers already inside a nested Claude group are left unchanged. +- Other targets do not receive the Claude-only default. +- Reinstalling over settings written before the fix leaves one typed group, + including content-matched healing of stale root-package entries. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from apm_cli.integration.hook_integrator import HookIntegrator +from apm_cli.integration.targets import KNOWN_TARGETS +from apm_cli.models.apm_package import APMPackage, PackageInfo + +pytestmark = pytest.mark.component + +_PACKAGE_NAME = "demo-pkg" + +# Exact flat hook file from the #3130 report: no ``type`` on the handler. +_FLAT_UNTYPED_HOOK = { + "version": 1, + "hooks": { + "preToolUse": [ + { + "command": 'node "${PLUGIN_ROOT}/.apm/hooks/scripts/gate.mjs"', + "matcher": "Write", + "timeout": 10, + } + ] + }, +} + +_CLAUDE_SHAPED_HOOK = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo native", "timeout": 5}], + } + ] + } +} + +_EXPECTED_FLAT_GROUP = { + "matcher": "Write", + "hooks": [ + { + "type": "command", + "command": ( + 'node "${CLAUDE_PROJECT_DIR}/.claude/hooks/demo-pkg/.apm/hooks/scripts/gate.mjs"' + ), + "timeout": 10, + } + ], +} + + +def _write_package(project_root: Path, hook_files: dict[str, dict]) -> PackageInfo: + """Materialise the #3130 package layout with the given hook files.""" + package_root = project_root / "apm_modules" / "owner" / _PACKAGE_NAME + hooks_dir = package_root / ".apm" / "hooks" + (hooks_dir / "scripts").mkdir(parents=True) + (hooks_dir / "scripts" / "gate.mjs").write_text("process.exit(0);\n", encoding="utf-8") + for name, document in hook_files.items(): + (hooks_dir / name).write_text(json.dumps(document), encoding="utf-8") + return PackageInfo( + package=APMPackage(name=_PACKAGE_NAME, version="0.0.1"), + install_path=package_root, + ) + + +def _integrate(project_root: Path, package_info: PackageInfo, target: str) -> None: + HookIntegrator().integrate_hooks_for_target( + KNOWN_TARGETS[target], + package_info, + project_root, + ) + + +def _read_json(path: Path) -> dict: + return json.loads(path.read_text(encoding="utf-8")) + + +def _without_ownership(entries: list) -> list: + """Drop APM ownership markers so sidecar entries compare to settings.""" + return [{k: v for k, v in entry.items() if k != "_apm_source"} for entry in entries] + + +def test_standalone_flat_untyped_entry_gets_command_type(tmp_path: Path) -> None: + (tmp_path / ".claude").mkdir() + package_info = _write_package(tmp_path, {"flat.json": _FLAT_UNTYPED_HOOK}) + + _integrate(tmp_path, package_info, "claude") + + settings = _read_json(tmp_path / ".claude" / "settings.json") + assert settings["hooks"] == {"PreToolUse": [_EXPECTED_FLAT_GROUP]} + sidecar = _read_json(tmp_path / ".claude" / "apm-hooks.json") + assert _without_ownership(sidecar["PreToolUse"]) == [_EXPECTED_FLAT_GROUP] + + +def test_flat_untyped_entry_merged_next_to_claude_shaped_file(tmp_path: Path) -> None: + (tmp_path / ".claude").mkdir() + package_info = _write_package( + tmp_path, + {"claude.json": _CLAUDE_SHAPED_HOOK, "flat.json": _FLAT_UNTYPED_HOOK}, + ) + + _integrate(tmp_path, package_info, "claude") + + settings = _read_json(tmp_path / ".claude" / "settings.json") + expected = [_CLAUDE_SHAPED_HOOK["hooks"]["PreToolUse"][0], _EXPECTED_FLAT_GROUP] + assert settings["hooks"] == {"PreToolUse": expected} + sidecar = _read_json(tmp_path / ".claude" / "apm-hooks.json") + assert _without_ownership(sidecar["PreToolUse"]) == expected + + +@pytest.mark.parametrize( + ("source_entry", "expected_handler"), + [ + pytest.param( + {"bash": "echo posix"}, + {"type": "command", "command": "echo posix"}, + id="untyped-bash-command", + ), + pytest.param( + {"type": "command", "command": "echo typed"}, + {"type": "command", "command": "echo typed"}, + id="explicit-command", + ), + pytest.param( + {"type": "prompt", "prompt": "Review the change"}, + {"type": "prompt", "prompt": "Review the change"}, + id="explicit-prompt", + ), + pytest.param( + {"foo": 1}, + {"foo": 1}, + id="untyped-without-command", + ), + ], +) +def test_flat_handler_types_are_defaulted_only_for_commands( + tmp_path: Path, source_entry: dict, expected_handler: dict +) -> None: + (tmp_path / ".claude").mkdir() + package_info = _write_package( + tmp_path, {"edge.json": {"hooks": {"PreToolUse": [source_entry]}}} + ) + + _integrate(tmp_path, package_info, "claude") + + settings = _read_json(tmp_path / ".claude" / "settings.json") + assert settings["hooks"]["PreToolUse"] == [{"matcher": "*", "hooks": [expected_handler]}] + + +def test_untyped_handler_inside_nested_claude_group_is_unchanged(tmp_path: Path) -> None: + (tmp_path / ".claude").mkdir() + nested = {"matcher": "Edit", "hooks": [{"command": "echo nested"}]} + package_info = _write_package(tmp_path, {"nested.json": {"hooks": {"PostToolUse": [nested]}}}) + + _integrate(tmp_path, package_info, "claude") + + settings = _read_json(tmp_path / ".claude" / "settings.json") + assert settings["hooks"] == {"PostToolUse": [nested]} + + +@pytest.mark.parametrize( + ("target", "config_dir", "config_file"), + [ + pytest.param("codex", ".codex", "hooks.json", id="codex"), + pytest.param("cursor", ".cursor", "hooks.json", id="cursor"), + ], +) +def test_claude_handler_default_does_not_reach_other_targets( + tmp_path: Path, target: str, config_dir: str, config_file: str +) -> None: + (tmp_path / config_dir).mkdir() + package_info = _write_package(tmp_path, {"flat.json": _FLAT_UNTYPED_HOOK}) + + _integrate(tmp_path, package_info, target) + + config = _read_json(tmp_path / config_dir / config_file) + rendered = json.dumps(config["hooks"]) + assert '"type"' not in rendered + assert "gate.mjs" in rendered + + +def test_reinstall_replaces_untyped_group_written_before_fix(tmp_path: Path) -> None: + claude_dir = tmp_path / ".claude" + claude_dir.mkdir() + package_info = _write_package(tmp_path, {"flat.json": _FLAT_UNTYPED_HOOK}) + legacy_group = { + "matcher": "Write", + "hooks": [ + { + "command": _EXPECTED_FLAT_GROUP["hooks"][0]["command"], + "timeout": 10, + } + ], + } + (claude_dir / "settings.json").write_text( + json.dumps({"hooks": {"PreToolUse": [legacy_group]}}), encoding="utf-8" + ) + (claude_dir / "apm-hooks.json").write_text( + json.dumps({"PreToolUse": [{**legacy_group, "_apm_source": _PACKAGE_NAME}]}), + encoding="utf-8", + ) + + _integrate(tmp_path, package_info, "claude") + _integrate(tmp_path, package_info, "claude") + + settings = _read_json(claude_dir / "settings.json") + assert settings["hooks"] == {"PreToolUse": [_EXPECTED_FLAT_GROUP]} + + +def test_stale_root_source_untyped_group_is_still_healed(tmp_path: Path) -> None: + """Content-matched healing must still recognise groups written before the fix.""" + (tmp_path / "apm.yml").write_text("name: consumer\nversion: 0.0.1\n", encoding="utf-8") + hooks_dir = tmp_path / ".apm" / "hooks" + hooks_dir.mkdir(parents=True) + (hooks_dir / "flat.json").write_text( + json.dumps({"hooks": {"PreToolUse": [{"command": "echo root", "matcher": "Write"}]}}), + encoding="utf-8", + ) + claude_dir = tmp_path / ".claude" + claude_dir.mkdir() + legacy_group = {"matcher": "Write", "hooks": [{"command": "echo root"}]} + (claude_dir / "settings.json").write_text( + json.dumps({"hooks": {"PreToolUse": [legacy_group]}}), encoding="utf-8" + ) + (claude_dir / "apm-hooks.json").write_text( + json.dumps({"PreToolUse": [{**legacy_group, "_apm_source": "old-root-name"}]}), + encoding="utf-8", + ) + root_package = PackageInfo( + package=APMPackage(name="consumer", version="0.0.1"), + install_path=tmp_path, + ) + + _integrate(tmp_path, root_package, "claude") + + settings = _read_json(claude_dir / "settings.json") + assert settings["hooks"] == { + "PreToolUse": [{"matcher": "Write", "hooks": [{"type": "command", "command": "echo root"}]}] + } From 3be1ab0586f98fe3e51e95f7648588b05444ff40 Mon Sep 17 00:00:00 2001 From: parth Date: Fri, 2 Oct 2026 22:51:29 +0530 Subject: [PATCH 2/4] docs(changelog): cite #3153 in the #3130 hook entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d954b38b10..bd935830b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- `apm install` now writes flat hook command entries that omit `type` to `.claude/settings.json` as schema-valid `"type": "command"` handlers, including when merged next to a Claude-shaped hook file. Explicit handler types and other targets are unchanged; reinstall to refresh existing entries. (by @Parth-Vasave, #3130) +- `apm install` now writes flat hook command entries that omit `type` to `.claude/settings.json` as schema-valid `"type": "command"` handlers, including when merged next to a Claude-shaped hook file. Explicit handler types and other targets are unchanged; reinstall to refresh existing entries. (by @Parth-Vasave, closes #3130, #3153) ## [0.33.0] - 2026-10-02 From f53c7218cf5f88189079d628ad78f968eb5584ec Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 16 Oct 2026 11:25:20 +0200 Subject: [PATCH 3/4] test(hooks): prove Claude typing through install and upgrade Fold panel coverage gaps for #3130: verify ownership sidecars, actual CLI install/reinstall and legacy root healing without executing hooks. Preserve non-target behavior without pinning invalid Codex output; add canonical-edge and explicit-type traps and clarify flat-only guidance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a --- CHANGELOG.md | 4 +- .../author-primitives/hooks-and-commands.md | 6 +- .../skills/apm-usage/package-authoring.md | 1 + .../test_architecture_contract_guards.py | 8 +- .../test_required_lifecycle_state_machine.py | 94 +++++++++++++++++++ .../test_hook_integrator_issue3130.py | 30 +++++- 6 files changed, 134 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5fb268aa4..1a236506b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Fixed - -- `apm install` now writes flat hook command entries that omit `type` to `.claude/settings.json` as schema-valid `"type": "command"` handlers, including when merged next to a Claude-shaped hook file. Explicit handler types and other targets are unchanged; reinstall to refresh existing entries. (by @Parth-Vasave, closes #3130, #3153) ### Added - Agent primitives (`*.agent.md`) accept an optional `user-invocable: false` frontmatter field (default `true`; `visibility: internal` is an accepted alias). APM preserves the metadata so the agent is intended to be reachable programmatically only (for example via `handoffs:` or subagent calls); picker exclusion and that reachability take effect only when the receiving harness supports them. Verbatim-copy targets preserve the field; Codex and Kiro warn instead of silently dropping it. -- by @richard-fowles-epam (#3132) @@ -23,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `apm install` now writes untyped flat command hooks as Claude `"type": "command"` handlers, including alongside Claude-shaped hook files. Explicit types and other targets are unchanged; reinstall to refresh existing entries. -- by @Parth-Vasave (#3153, closes #3130) - `apm install` now reconciles renamed or deleted project-local skills and removed targets using recorded concrete ownership, preserving ambiguous legacy output, shared claims, and user edits -- by @tillig (#3190) - `apm install plugin@mkt` for an in-repository plugin of a `github.com` marketplace registered over SSH pre-flights the repository over SSH instead of HTTPS-only probes, so a private marketplace installs with an SSH key and no GitHub token. (#3164) - `apm pack --check-clean` now certifies GitHub-hosted manifestless skills when `SKILL.md` has valid frontmatter at the resolved repository, ref, and subdirectory, while invalid skill metadata and missing repositories, refs, and package paths remain fail-closed. (#3064) diff --git a/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md b/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md index fdc905412c..2a5a33e32c 100644 --- a/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md +++ b/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md @@ -198,10 +198,10 @@ integrator renders its native schema. Flat command entries become Claude's required `{ "matcher": "*", "hooks": [...] }` entries in `.claude/settings.json`; a flat command entry that omits `type` receives Claude's required `"type": "command"`, while explicit handler types are kept -and other targets receive no default. For Codex, flat command entries become +and other targets are unchanged. This default applies only to flat command +entries, not handlers already inside nested Claude groups. For Codex, flat command entries become intermediate hook groups containing a nested `hooks` array in -`.codex/hooks.json`. Kiro -receives its current v1 standalone schema: +`.codex/hooks.json`. Kiro receives its current v1 standalone schema: `{ "version": "v1", "hooks": [{ "name", "trigger", "matcher", "action" }] }`. Kiro trigger names are PascalCase and command timeouts remain in seconds. 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 6242268acd..28b1b972ab 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -184,6 +184,7 @@ hook action under `.kiro/hooks/`. For Claude, APM wraps flat command entries in `{ "matcher", "hooks": [...] }` groups in `.claude/settings.json` and adds the required `"type": "command"` when a flat command entry omits `type`. Explicit handler types are kept. +Handlers already inside nested Claude groups and other targets are unchanged. For Codex, APM wraps flat command entries in hook groups containing a nested `hooks` array in `.codex/hooks.json`. diff --git a/tests/integration/test_architecture_contract_guards.py b/tests/integration/test_architecture_contract_guards.py index e57fc82a62..f1522c15ba 100644 --- a/tests/integration/test_architecture_contract_guards.py +++ b/tests/integration/test_architecture_contract_guards.py @@ -119,13 +119,19 @@ def test_codex_native_edge_preserves_nested_entries() -> None: assert _to_codex_hook_entries(source) == source -def test_claude_edge_nests_flat_portable_hook_entries(tmp_path: Path) -> None: +@pytest.mark.parametrize("typed", [True, False], ids=["explicit-type", "default-type"]) +def test_claude_edge_nests_flat_portable_hook_entries(tmp_path: Path, typed: bool) -> None: """Claude output must wrap portable handlers in matcher/hooks entries.""" from apm_cli.integration.hook_integrator import HookIntegrator from apm_cli.integration.targets import KNOWN_TARGETS (tmp_path / ".claude").mkdir() package_info = _write_portable_hook_package(tmp_path) + if not typed: + hook_path = package_info.install_path / ".apm/hooks/check.json" + hook = json.loads(hook_path.read_text(encoding="utf-8")) + hook["hooks"]["PreToolUse"][0].pop("type") + hook_path.write_text(json.dumps(hook), encoding="utf-8") HookIntegrator().integrate_hooks_for_target( KNOWN_TARGETS["claude"], diff --git a/tests/integration/test_required_lifecycle_state_machine.py b/tests/integration/test_required_lifecycle_state_machine.py index 1102863b26..b9d6622126 100644 --- a/tests/integration/test_required_lifecycle_state_machine.py +++ b/tests/integration/test_required_lifecycle_state_machine.py @@ -1726,6 +1726,100 @@ def test_required_cursor_metadata_survives_install_compile_update( assert (consumer.root / relative).read_bytes() == data +@pytest.mark.parametrize("with_native_group", [False, True], ids=["standalone", "merged"]) +def test_required_claude_untyped_hook_upgrade_preserves_ownership_and_converges( + tmp_path: Path, apm_binary_path: Path, with_native_group: bool +) -> None: + """Real installs heal pre-fix handlers without executing or claiming user hooks.""" + scenario = _new_scenario(tmp_path / "claude-untyped", apm_binary_path) + source = _publish(scenario, "flat-kit", hook_command="echo native") + hook_root = source.repository.worktree / ".apm" / "hooks" + if not with_native_group: + (hook_root / "pretool.json").unlink() + command = "echo must-not-run > hook-executed" + flat = {"command": command, "matcher": "Write", "timeout": 10} + (hook_root / "flat.json").write_text( + json.dumps({"hooks": {"preToolUse": [flat]}}), encoding="ascii" + ) + commit = scenario.repositories.commit(source.repository, message="publish untyped flat hook") + consumer = scenario.consumers.create( + "claude-consumer", + dependencies=({**source.dependency, "ref": commit.sha},), + targets=("claude",), + ) + scenario.consumers.add_hook( + consumer, "root", {"hooks": {"PreToolUse": [{"command": "echo root"}]}} + ) + settings_path = consumer.root / ".claude/settings.json" + sidecar_path = consumer.root / ".claude/apm-hooks.json" + settings_path.parent.mkdir() + user_group = {"matcher": "Read", "hooks": [{"type": "prompt", "prompt": "Keep user hook"}]} + settings_path.write_text( + json.dumps( + {"permissions": {"deny": ["Bash(rm:*)"]}, "hooks": {"PreToolUse": [user_group]}} + ), + encoding="ascii", + ) + sentinel = consumer.root / ".codex/hooks.json" + sentinel.parent.mkdir() + sentinel.write_bytes(b'{"hooks": {}, "user": "unchanged"}\n') + authored = ArtifactSnapshot.capture(consumer.root / ".apm") + + _run_success( + scenario, + consumer, + (*_INSTALL_ARGS, "--target", "claude"), + environment=source.environment, + scenario_id="claude-untyped-install", + ) + expected_settings = json.loads(settings_path.read_text(encoding="utf-8")) + expected_sidecar = json.loads(sidecar_path.read_text(encoding="utf-8")) + groups = expected_settings["hooks"]["PreToolUse"] + handlers = [handler for group in groups for handler in group["hooks"]] + assert {"type": "command", "command": command, "timeout": 10} in handlers + assert {"type": "command", "command": "echo root"} in handlers + assert all(handler["type"] in {"command", "prompt"} for handler in handlers) + assert user_group in groups + assert len(groups) == 3 + int(with_native_group) + assert {entry["_apm_source"] for entry in expected_sidecar["PreToolUse"]} == { + f"{_OWNER}/flat-kit", + "_local/claude-consumer", + } + assert all(entry["hooks"] != user_group["hooks"] for entry in expected_sidecar["PreToolUse"]) + + # Recreate the previous renderer's untyped output and a renamed root source. + legacy_settings = json.loads(settings_path.read_text(encoding="utf-8")) + legacy_sidecar = json.loads(sidecar_path.read_text(encoding="utf-8")) + for entries in (legacy_settings["hooks"]["PreToolUse"], legacy_sidecar["PreToolUse"]): + for entry in entries: + for handler in entry["hooks"]: + if handler.get("command") in {command, "echo root"}: + handler.pop("type") + if entry.get("_apm_source") == "_local/claude-consumer": + entry["_apm_source"] = "old-checkout-name" + settings_path.write_text(json.dumps(legacy_settings), encoding="ascii") + sidecar_path.write_text(json.dumps(legacy_sidecar), encoding="ascii") + + baseline = None + for cycle in range(2): + _run_success( + scenario, + consumer, + (*_INSTALL_ARGS, "--target", "claude"), + environment=source.environment, + scenario_id=f"claude-untyped-reinstall-{cycle}", + ) + assert json.loads(settings_path.read_text(encoding="utf-8")) == expected_settings + assert json.loads(sidecar_path.read_text(encoding="utf-8")) == expected_sidecar + assert sentinel.read_bytes() == b'{"hooks": {}, "user": "unchanged"}\n' + assert not (consumer.root / "hook-executed").exists() + assert_unchanged(authored, ArtifactSnapshot.capture(consumer.root / ".apm")) + snapshot = LifecycleStateSnapshot.capture(consumer.root, targets=("claude",)) + if baseline is not None: + _assert_same_state(baseline, snapshot) + baseline = snapshot + + @pytest.mark.parametrize("alias", [".safe", "safe.", "foo..bar", "my-skill.v2"]) def test_required_reinstall_is_byte_idempotent_across_durable_state( tmp_path: Path, diff --git a/tests/unit/integration/test_hook_integrator_issue3130.py b/tests/unit/integration/test_hook_integrator_issue3130.py index 5795d85964..cd01979f56 100644 --- a/tests/unit/integration/test_hook_integrator_issue3130.py +++ b/tests/unit/integration/test_hook_integrator_issue3130.py @@ -21,6 +21,7 @@ import json from pathlib import Path +from unittest.mock import patch import pytest @@ -138,6 +139,16 @@ def test_flat_untyped_entry_merged_next_to_claude_shaped_file(tmp_path: Path) -> {"type": "command", "command": "echo posix"}, id="untyped-bash-command", ), + pytest.param( + {"powershell": "Write-Output windows"}, + {"type": "command", "command": "Write-Output windows"}, + id="untyped-powershell-command", + ), + pytest.param( + {"windows": "echo windows"}, + {"type": "command", "command": "echo windows"}, + id="untyped-windows-command", + ), pytest.param( {"type": "command", "command": "echo typed"}, {"type": "command", "command": "echo typed"}, @@ -148,6 +159,11 @@ def test_flat_untyped_entry_merged_next_to_claude_shaped_file(tmp_path: Path) -> {"type": "prompt", "prompt": "Review the change"}, id="explicit-prompt", ), + pytest.param( + {"type": "prompt", "command": "echo explicit", "prompt": "Review"}, + {"type": "prompt", "command": "echo explicit", "prompt": "Review"}, + id="explicit-prompt-with-command", + ), pytest.param( {"foo": 1}, {"foo": 1}, @@ -193,11 +209,15 @@ def test_claude_handler_default_does_not_reach_other_targets( (tmp_path / config_dir).mkdir() package_info = _write_package(tmp_path, {"flat.json": _FLAT_UNTYPED_HOOK}) - _integrate(tmp_path, package_info, target) + with patch( + "apm_cli.integration.hook_native_formats._with_claude_default_handler_type", + side_effect=AssertionError("Claude handler typing reached another target"), + ) as claude_default: + _integrate(tmp_path, package_info, target) + claude_default.assert_not_called() config = _read_json(tmp_path / config_dir / config_file) rendered = json.dumps(config["hooks"]) - assert '"type"' not in rendered assert "gate.mjs" in rendered @@ -227,6 +247,8 @@ def test_reinstall_replaces_untyped_group_written_before_fix(tmp_path: Path) -> settings = _read_json(claude_dir / "settings.json") assert settings["hooks"] == {"PreToolUse": [_EXPECTED_FLAT_GROUP]} + sidecar = _read_json(claude_dir / "apm-hooks.json") + assert sidecar == {"PreToolUse": [{**_EXPECTED_FLAT_GROUP, "_apm_source": _PACKAGE_NAME}]} def test_stale_root_source_untyped_group_is_still_healed(tmp_path: Path) -> None: @@ -259,3 +281,7 @@ def test_stale_root_source_untyped_group_is_still_healed(tmp_path: Path) -> None assert settings["hooks"] == { "PreToolUse": [{"matcher": "Write", "hooks": [{"type": "command", "command": "echo root"}]}] } + sidecar = _read_json(claude_dir / "apm-hooks.json") + assert sidecar == { + "PreToolUse": [{**settings["hooks"]["PreToolUse"][0], "_apm_source": "_local/consumer"}] + } From bbd89bf3e4ea5ff9ad59ea86f4e34a9203285b77 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 16 Oct 2026 11:30:01 +0200 Subject: [PATCH 4/4] refactor(hooks): share legacy native shapes for cleanup Keep the integrator below the canonical module limit by consolidating the duplicate legacy-key selection through the existing native renderer owner. Preserve Codex flat legacy matching and Claude untyped upgrade healing. Add a static boundary assertion and mutation-proven functional guardrail. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a --- .../checks/mutation_hook_contract.py | 17 +++++++++++++++++ src/apm_cli/integration/hook_integrator.py | 19 ++++++------------- .../integration/hook_native_formats.py | 8 ++++++++ .../test_architecture_contract_guards.py | 17 +++++++++++++++++ 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/scripts/architecture_linter/checks/mutation_hook_contract.py b/scripts/architecture_linter/checks/mutation_hook_contract.py index 3bfb79db4d..b53f5d1d87 100644 --- a/scripts/architecture_linter/checks/mutation_hook_contract.py +++ b/scripts/architecture_linter/checks/mutation_hook_contract.py @@ -405,6 +405,7 @@ def _check_neutral_hook_contract(provider: FactsProvider) -> Iterable[Violation] *_nhc_cursor_preflight_unconditional(provider, rule_id), *_nhc_contract_vocabulary(provider, rule_id), *_nhc_command_keys(provider, rule_id), + *_nhc_legacy_native_shapes(provider, rule_id), *_nhc_file_routing(provider, rule_id), *_nhc_config_writes(provider, rule_id), *_nhc_ownership_markers(provider, rule_id), @@ -720,6 +721,22 @@ def _nhc_command_keys(provider: FactsProvider, rule_id: str) -> tuple[Violation, return tuple(findings) +def _nhc_legacy_native_shapes(provider: FactsProvider, rule_id: str) -> tuple[Violation, ...]: + """Ownership matching must obtain legacy shapes from the native renderer.""" + facts, failures = checked_facts(provider, _HOOK_INTEGRATOR, rule_id, require_python=True) + if failures: + return failures + return _require( + any( + call.scope == "_integrate_merged_hooks" and call.qualname == "_legacy_hook_entries" + for call in facts.calls + ), + rule_id, + _HOOK_INTEGRATOR, + "legacy hook shapes must route through _legacy_hook_entries", + ) + + def _nhc_file_routing(provider: FactsProvider, rule_id: str) -> tuple[Violation, ...]: """Per-file hook routing must not be gated by ``dep_targets_active``.""" findings: list[Violation] = [] diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 8a6a2495e7..04a9af5251 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -48,6 +48,7 @@ from apm_cli.integration.hook_cursor_preflight import preflight_cursor_hooks from apm_cli.integration.hook_file_routing import filter_hook_files_for_target from apm_cli.integration.hook_native_formats import ( + _legacy_hook_entries, _to_antigravity_hook_entries, _to_claude_hook_entries, _to_codex_hook_entries, @@ -1362,26 +1363,18 @@ def _integrate_merged_hooks( event_name = event_map.get(raw_event_name, raw_event_name) if event_name not in json_config[container]: json_config[container][event_name] = [] - legacy_content_keys: set[str] = set() + legacy_content_keys = { + self._hook_entry_content_key(entry) + for entry in _legacy_hook_entries(entries, config.target_key) + if isinstance(entry, dict) + } if config.target_key == "claude": - # Match owned untyped groups from installs before handler typing. - legacy_content_keys = { - self._hook_entry_content_key(entry) - for entry in _to_claude_hook_entries(entries, default_handler_type=False) - if isinstance(entry, dict) - } entries = _to_claude_hook_entries(entries) elif config.target_key == "cursor": entries = _to_cursor_hook_entries( entries, event_name, foreign=raw_event_name in event_map ) elif config.target_key == "codex": - # Match owned flat entries from installs before Codex nesting. - legacy_content_keys = { - self._hook_entry_content_key(entry) - for entry in entries - if isinstance(entry, dict) - } entries = _to_codex_hook_entries(entries) elif config.target_key == "gemini": entries = _to_gemini_hook_entries(entries) diff --git a/src/apm_cli/integration/hook_native_formats.py b/src/apm_cli/integration/hook_native_formats.py index 10c4902c0c..6e14b4e6c1 100644 --- a/src/apm_cli/integration/hook_native_formats.py +++ b/src/apm_cli/integration/hook_native_formats.py @@ -411,6 +411,14 @@ def _to_gemini_hook_entries(entries: list) -> list: ) +def _legacy_hook_entries(entries: list, target: str) -> list: + """Render old native shapes for ownership matching, never for deployment.""" + if target == "claude": + return _to_claude_hook_entries(entries, default_handler_type=False) + # Codex installs before native nesting wrote the original flat entries. + return entries if target == "codex" else [] + + def _with_claude_default_handler_type(entry: object) -> object: """Supply Claude's required ``type`` for an untyped flat command entry. diff --git a/tests/integration/test_architecture_contract_guards.py b/tests/integration/test_architecture_contract_guards.py index f1522c15ba..8fdf1fa299 100644 --- a/tests/integration/test_architecture_contract_guards.py +++ b/tests/integration/test_architecture_contract_guards.py @@ -12,6 +12,23 @@ _RULES_BY_ID = {rule.id: rule for rule in registered_rules()} +def test_legacy_hook_shapes_boundary_rejects_cleanup_bypass() -> None: + """A second legacy-shape policy cannot silently bypass the native owner.""" + root = Path(__file__).parents[2] + path = "src/apm_cli/integration/hook_integrator.py" + source = (root / path).read_text(encoding="utf-8") + old = "_legacy_hook_entries(entries, config.target_key)" + assert old in source + rule_id = "mutation_writes.neutral_hook_contract" + result = run_selected_rules( + root, (rule_id,), source_overrides={path: source.replace(old, "[]", 1)} + ) + assert any( + finding.rule_id == rule_id and "_legacy_hook_entries" in finding.message + for finding in result.violations + ) + + @pytest.mark.parametrize( ("path", "old", "new"), [