diff --git a/CHANGELOG.md b/CHANGELOG.md index a06ad8bb9e..c230709888 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` and `apm update` no longer fail with `Object of type mappingproxy is not JSON serializable` when a hook carries nested metadata such as `env`; Claude, Codex, Gemini and Antigravity hook files keep that metadata as plain JSON. (by @abhishekacharya200, with regression coverage from @MohammedAlkindi in #3199, closes #3167) (#3211) + ## [0.34.0] - 2026-10-16 ### 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 f36d3c4160..30d384d4ef 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 @@ -201,6 +201,9 @@ 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. +For Claude, Codex, Gemini, and Antigravity, other handler and hook-group +fields, including nested objects and arrays such as `env`, are written to the +native JSON unchanged. ### Cursor native hooks and Claude import diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index 5f6f9d6a63..35f0b9857a 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -183,6 +183,8 @@ hook action under `.kiro/hooks/`. For Codex, APM wraps flat command entries in hook groups containing a nested `hooks` array in `.codex/hooks.json`. +Claude, Codex, Gemini, and Antigravity preserve nested handler and +hook-group metadata, including `env` objects and arrays, in native JSON. For Cursor, APM writes native v1 flat command or prompt handlers using only verified aliases. Unsupported fields, restrictions and overlapping Claude diff --git a/scripts/architecture_linter/checks/mutation_hook_contract.py b/scripts/architecture_linter/checks/mutation_hook_contract.py index f23a321c2b..0fb2839351 100644 --- a/scripts/architecture_linter/checks/mutation_hook_contract.py +++ b/scripts/architecture_linter/checks/mutation_hook_contract.py @@ -409,6 +409,7 @@ def _check_neutral_hook_contract(provider: FactsProvider) -> Iterable[Violation] *_nhc_config_writes(provider, rule_id), *_nhc_ownership_markers(provider, rule_id), *_nhc_drift_projection(provider, rule_id), + *_nhc_native_metadata_thaw(provider, rule_id), ) @@ -552,6 +553,47 @@ def _nhc_contract_vocabulary(provider: FactsProvider, rule_id: str) -> tuple[Vio ) +def _nhc_native_metadata_thaw(provider: FactsProvider, rule_id: str) -> tuple[Violation, ...]: + """Native renderers must thaw frozen hook metadata through ``hook_contract._thaw``. + + The IR freezes nested metadata, so a shallow ``dict(...)`` copy or a raw + ``raw_entry`` passthrough leaves nested mappings frozen and breaks JSON + serialization (#3167). + """ + renderer = "src/apm_cli/integration/hook_native_formats.py" + facts_by_path, failures = _read_required(provider, rule_id, (_HOOK_CONTRACT, renderer)) + findings: list[Violation] = list(failures) + if not failures: + findings.extend( + _require( + _count_regex_lines(facts_by_path[_HOOK_CONTRACT], r"^def _thaw\(") == 1, + rule_id, + _HOOK_CONTRACT, + "_thaw must be defined exactly once by hook_contract", + ) + ) + for required in ("_thaw(handler.metadata)", "_thaw(binding.metadata)"): + findings.extend( + _require( + _has_fixed(facts_by_path[renderer], required), + rule_id, + renderer, + f"native hook renderers must call {required}", + ) + ) + findings.extend( + line_pattern_violations( + provider, + rule_id=rule_id, + paths=(renderer,), + pattern=r'dict\((?:handler|binding)\.metadata\)|append\(binding\.metadata\["raw_entry"\]\)', + message="native hook renderers must thaw frozen metadata with hook_contract._thaw", + exempt_marker=EXEMPT_MARKER, + ) + ) + return tuple(findings) + + def _nhc_cursor_edge(provider: FactsProvider, rule_id: str) -> tuple[Violation, ...]: """Keep Cursor rendering and import preflight on the canonical native edge.""" renderer = "src/apm_cli/integration/hook_native_formats.py" diff --git a/src/apm_cli/hook_contract.py b/src/apm_cli/hook_contract.py index 284d46845e..c30cf09315 100644 --- a/src/apm_cli/hook_contract.py +++ b/src/apm_cli/hook_contract.py @@ -32,6 +32,15 @@ def _freeze(value: Any) -> Any: return value +def _thaw(value: Any) -> Any: + """Recursively copy frozen metadata into independent JSON-compatible values.""" + if isinstance(value, Mapping): + return {key: _thaw(item) for key, item in value.items()} + if isinstance(value, (list, tuple)): + return [_thaw(item) for item in value] + return value + + @dataclass(frozen=True) class HookHandler: """One portable command handler.""" diff --git a/src/apm_cli/integration/hook_native_formats.py b/src/apm_cli/integration/hook_native_formats.py index 05e931628f..02956ae71d 100644 --- a/src/apm_cli/integration/hook_native_formats.py +++ b/src/apm_cli/integration/hook_native_formats.py @@ -13,6 +13,7 @@ HookHandler, _entries_to_ir, _handler_to_ir, + _thaw, hook_handlers, ) @@ -350,7 +351,7 @@ def inspect_native_hooks( def _handler_from_ir(handler: HookHandler, *, timeout_milliseconds: bool) -> dict[str, Any]: """Render a portable handler into one native command object.""" - result = dict(handler.metadata) + result = _thaw(handler.metadata) if handler.command is not None: result["command"] = handler.command if handler.timeout_seconds is not None: @@ -372,9 +373,9 @@ def _render_nested_document( result: list = [] for binding in document.bindings: if "raw_entry" in binding.metadata: - result.append(binding.metadata["raw_entry"]) + result.append(_thaw(binding.metadata["raw_entry"])) continue - outer = dict(binding.metadata) + outer = _thaw(binding.metadata) if binding.matcher is not None or default_matcher is not None: outer["matcher"] = binding.matcher or default_matcher outer["hooks"] = [ @@ -441,7 +442,7 @@ def _to_antigravity_hook_entries(entries: list, event_name: str) -> list: flat: list[dict[str, Any]] = [] for binding in document.bindings: if "raw_entry" in binding.metadata: - flat.append(binding.metadata["raw_entry"]) + flat.append(_thaw(binding.metadata["raw_entry"])) continue for handler in binding.handlers: rendered = _handler_from_ir(handler, timeout_milliseconds=False) diff --git a/tests/integration/test_architecture_contract_guards.py b/tests/integration/test_architecture_contract_guards.py index e57fc82a62..ae3faf8bf5 100644 --- a/tests/integration/test_architecture_contract_guards.py +++ b/tests/integration/test_architecture_contract_guards.py @@ -50,7 +50,43 @@ def test_cursor_native_edge_boundary_rejects_bypass_and_second_owner( assert any(finding.rule_id == rule.id for finding in result.violations) -def _write_portable_hook_package(tmp_path: Path) -> object: +@pytest.mark.parametrize( + ("old", "new"), + [ + ("result = _thaw(handler.metadata)", "result = dict(handler.metadata)"), + ("outer = _thaw(binding.metadata)", "outer = dict(binding.metadata)"), + ( + 'result.append(_thaw(binding.metadata["raw_entry"]))', + 'result.append(binding.metadata["raw_entry"])', + ), + ( + 'flat.append(_thaw(binding.metadata["raw_entry"]))', + 'flat.append(binding.metadata["raw_entry"])', + ), + ], +) +def test_native_hook_renderers_reject_shallow_metadata_copies(old: str, new: str) -> None: + """Restoring a shallow metadata copy at a render boundary must fail the guard (#3167).""" + root = Path(__file__).parents[2] + path = "src/apm_cli/integration/hook_native_formats.py" + source = (root / path).read_text(encoding="utf-8") + assert old in source + rule = _RULES_BY_ID["mutation_writes.neutral_hook_contract"] + result = run_selected_rules( + root, (rule.id,), source_overrides={path: source.replace(old, new, 1)} + ) + assert any(finding.rule_id == rule.id for finding in result.violations) + + +def test_neutral_hook_contract_passes_on_current_sources() -> None: + """The unmodified tree satisfies the neutral hook contract rule.""" + root = Path(__file__).parents[2] + rule = _RULES_BY_ID["mutation_writes.neutral_hook_contract"] + result = run_selected_rules(root, (rule.id,)) + assert [finding for finding in result.violations if finding.rule_id == rule.id] == [] + + +def _write_portable_hook_package(tmp_path: Path, *, nested_metadata: bool = False) -> object: """Create one package carrying the flat portable hook shape.""" from apm_cli.models.apm_package import APMPackage, PackageInfo @@ -67,6 +103,7 @@ def _write_portable_hook_package(tmp_path: Path) -> object: "command": "echo check", "timeout": 10, "matcher": "fs_write|str_replace", + **(_NESTED_HOOK_METADATA if nested_metadata else {}), } ] } @@ -119,6 +156,147 @@ def test_codex_native_edge_preserves_nested_entries() -> None: assert _to_codex_hook_entries(source) == source +_NESTED_HOOK_METADATA = { + "env": {"APM_TEST": "value", "PATHS": ["a", "b"]}, + "x-extension": {"tags": [{"name": "t"}], "enabled": True, "weight": 1.5, "note": None}, +} + + +def _rendered_handler(target: str) -> tuple[object, dict]: + """Render one portable handler carrying nested metadata for one native edge.""" + from apm_cli.integration.hook_native_formats import ( + _to_antigravity_hook_entries, + _to_claude_hook_entries, + _to_codex_hook_entries, + _to_gemini_hook_entries, + ) + + source = [{"command": "echo ok", "timeoutSec": 3, **_NESTED_HOOK_METADATA}] + if target == "antigravity-flat": + rendered = _to_antigravity_hook_entries(source, "PreInvocation") + return rendered, rendered[0] + renderers = { + "claude": _to_claude_hook_entries, + "codex": _to_codex_hook_entries, + "gemini": _to_gemini_hook_entries, + "antigravity-nested": lambda entries: _to_antigravity_hook_entries(entries, "PreToolUse"), + } + rendered = renderers[target](source) + return rendered, rendered[0]["hooks"][0] + + +@pytest.mark.parametrize( + ("target", "timeout"), + [ + ("claude", 3), + ("codex", 3), + ("gemini", 3000), + ("antigravity-nested", 3), + ("antigravity-flat", 3), + ], +) +def test_native_edges_render_nested_hook_metadata_as_json(target: str, timeout: int) -> None: + """Nested hook metadata must reach native JSON documents intact (#3167).""" + rendered, handler = _rendered_handler(target) + + assert json.loads(json.dumps(rendered)) == rendered + assert handler["command"] == "echo ok" + assert handler["timeout"] == timeout + assert handler["env"] == _NESTED_HOOK_METADATA["env"] + assert handler["x-extension"] == _NESTED_HOOK_METADATA["x-extension"] + + +def test_native_edges_render_nested_binding_metadata_and_raw_entries_as_json() -> None: + """Matcher-group metadata and raw entries must round-trip through JSON.""" + from apm_cli.integration.hook_native_formats import ( + _to_antigravity_hook_entries, + _to_codex_hook_entries, + ) + + nested = [ + { + "matcher": "Bash", + "x-group": {"labels": ["a", {"b": 1}]}, + "hooks": [{"type": "command", "command": "echo check", "env": {"K": "v"}}], + } + ] + raw = [["opaque", {"inner": {"value": 1}}]] + + for rendered, expected in ( + (_to_codex_hook_entries(nested), nested), + (_to_codex_hook_entries(raw), raw), + (_to_antigravity_hook_entries(raw, "PreInvocation"), raw), + ): + assert json.loads(json.dumps(rendered)) == expected + + +def test_rendered_hook_metadata_is_independent_of_the_frozen_ir() -> None: + """Mutating native output must not reach the immutable hook IR.""" + from apm_cli.hook_contract import _entries_to_ir + from apm_cli.integration.hook_native_formats import _handler_from_ir + + document = _entries_to_ir([{"command": "echo ok", **_NESTED_HOOK_METADATA}]) + handler = document.bindings[0].handlers[0] + + rendered = _handler_from_ir(handler, timeout_milliseconds=False) + rendered["env"]["APM_TEST"] = "changed" + rendered["env"]["PATHS"].append("c") + + assert handler.metadata["env"]["APM_TEST"] == "value" + assert tuple(handler.metadata["env"]["PATHS"]) == ("a", "b") + assert ( + _handler_from_ir(handler, timeout_milliseconds=False)["env"] == _NESTED_HOOK_METADATA["env"] + ) + + +def test_gemini_compatibility_conversion_preserves_nested_env() -> None: + """The in-place Gemini key conversion must keep nested env as plain JSON.""" + from apm_cli.integration.hook_native_formats import _copilot_keys_to_gemini + + hook = {"bash": "echo ok", "timeoutSec": 2, "env": {"APM_TEST": "value"}} + _copilot_keys_to_gemini(hook) + + assert hook["env"] == {"APM_TEST": "value"} + assert hook["timeout"] == 2000 + assert json.loads(json.dumps(hook)) == hook + + +@pytest.mark.parametrize( + ("target", "namespace", "event", "timeout"), + [ + ("claude", "hooks", "PreToolUse", 10), + ("codex", "hooks", "PreToolUse", 10), + ("gemini", "hooks", "BeforeTool", 10000), + ("antigravity", "apm", "PreToolUse", 10), + ], +) +def test_integrator_writes_nested_hook_metadata_to_native_json( + tmp_path: Path, target: str, namespace: str, event: str, timeout: int +) -> None: + """Real native file writes must preserve nested metadata without executing hooks.""" + from apm_cli.integration.hook_integrator import HookIntegrator + from apm_cli.integration.targets import KNOWN_TARGETS + + profile = KNOWN_TARGETS[target] + (tmp_path / profile.root_dir).mkdir() + package_info = _write_portable_hook_package(tmp_path, nested_metadata=True) + source = package_info.install_path / ".apm" / "hooks" / "check.json" + original = source.read_bytes() + + result = HookIntegrator().integrate_hooks_for_target(profile, package_info, tmp_path) + + assert result.files_integrated == 1 + payload = json.loads((tmp_path / profile.hooks_config_display).read_text(encoding="utf-8")) + binding = payload[namespace][event][0] + handler = binding["hooks"][0] + assert binding["matcher"] == "fs_write|str_replace" + assert handler["command"] == "echo check" + assert handler["timeout"] == timeout + assert handler["env"] == _NESTED_HOOK_METADATA["env"] + assert handler["x-extension"] == _NESTED_HOOK_METADATA["x-extension"] + assert source.read_bytes() == original + + def test_claude_edge_nests_flat_portable_hook_entries(tmp_path: Path) -> None: """Claude output must wrap portable handlers in matcher/hooks entries.""" from apm_cli.integration.hook_integrator import HookIntegrator diff --git a/tests/integration/test_hook_nested_metadata_install.py b/tests/integration/test_hook_nested_metadata_install.py new file mode 100644 index 0000000000..cb6c9225f7 --- /dev/null +++ b/tests/integration/test_hook_nested_metadata_install.py @@ -0,0 +1,99 @@ +"""Local install/update regression adapted from #3199 without policy bypass. + +Click drives the normal approval and install pipeline; no hook executes. +""" + +from __future__ import annotations + +import json +import sys +from pathlib import Path + +import pytest +import yaml +from click.testing import CliRunner + +from apm_cli.cli import cli +from apm_cli.models.apm_package import clear_apm_yml_cache + +pytestmark = [pytest.mark.component, pytest.mark.integration] + + +def test_install_update_write_nested_hook_metadata( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Normal local approval preserves nested JSON on install and update.""" + package = tmp_path / "hookpkg" + hooks = package / ".apm" / "hooks" + hooks.mkdir(parents=True) + (package / "apm.yml").write_text("name: nested-env\nversion: 1.0.0\n", encoding="utf-8") + sentinel = tmp_path / "HOOK_EXECUTED" + script = tmp_path / "must_not_run.py" + script.write_text( + f"from pathlib import Path\nPath({str(sentinel)!r}).write_text('executed')\n", + encoding="utf-8", + ) + command = f'"{sys.executable}" "{script}"' + project = tmp_path / "project" + project.mkdir() + (project / "apm.yml").write_text( + yaml.safe_dump( + { + "name": "nested-env-consumer", + "version": "1.0.0", + "targets": ["claude", "codex"], + "dependencies": {"apm": ["../hookpkg"]}, + "executables": {"allow": {}}, + }, + sort_keys=False, + ), + encoding="utf-8", + ) + monkeypatch.chdir(project) + monkeypatch.setattr("apm_cli.commands._helpers.check_for_updates", lambda *_a, **_k: None) + runner = CliRunner() + + for operation in ("install", "update"): + env = {"APM_TEST": "value", "NESTED": {"LIST": ["a", {"b": 1}]}} + handler = { + "type": "command", + "command": command, + "timeout": 5, + "env": env, + "x-extension": {"enabled": True, "weight": 1.5, "note": None}, + } + source = hooks / "pre.json" + source.write_text( + json.dumps({"hooks": {"PreToolUse": [{"matcher": "Bash", "hooks": [handler]}]}}), + encoding="utf-8", + ) + original = source.read_bytes() + clear_apm_yml_cache() + if operation == "install": + pending = runner.invoke(cli, ["install"], catch_exceptions=False) + assert pending.exit_code == 0, pending.output + assert not (project / ".claude" / "settings.json").exists() + assert not (project / ".codex" / "hooks.json").exists() + assert not sentinel.exists() + approval = runner.invoke(cli, ["approve", "../hookpkg"], catch_exceptions=False) + assert approval.exit_code == 0, approval.output + manifest = yaml.safe_load((project / "apm.yml").read_text(encoding="utf-8")) + assert manifest["executables"]["allow"]["../hookpkg#1.0.0"]["hooks"] is True + + if operation == "update": + # An unchanged local lock is a no-op; exercise update's real write path. + (project / "apm.lock.yaml").unlink() + (project / ".claude" / "settings.json").unlink() + (project / ".codex" / "hooks.json").unlink() + args = ["update", "--yes"] if operation == "update" else ["install"] + result = runner.invoke(cli, args, catch_exceptions=False) + + assert result.exit_code == 0, result.output + for config in (".claude/settings.json", ".codex/hooks.json"): + document = json.loads((project / config).read_text(encoding="utf-8")) + [entry] = document["hooks"]["PreToolUse"] + assert entry["matcher"] == "Bash", config + [rendered] = entry["hooks"] + assert rendered == handler, config + assert source.read_bytes() == original + assert not sentinel.exists() diff --git a/tests/unit/integration/test_hook_native_metadata.py b/tests/unit/integration/test_hook_native_metadata.py new file mode 100644 index 0000000000..747691435d --- /dev/null +++ b/tests/unit/integration/test_hook_native_metadata.py @@ -0,0 +1,122 @@ +"""Binding, provenance and isolation regressions adapted from #3199.""" + +from __future__ import annotations + +import copy +import json +from collections.abc import Callable +from typing import Any + +import pytest + +from apm_cli.hook_contract import _entries_to_ir +from apm_cli.integration.hook_native_formats import ( + _render_nested_document, + _to_antigravity_hook_entries, + _to_claude_hook_entries, + _to_codex_hook_entries, + _to_gemini_hook_entries, +) + +pytestmark = pytest.mark.unit + +_ENV = {"APM_TEST": "value", "NESTED": {"LEVEL": ["a", {"b": 1}]}} +_ARGS = ["--flag", {"key": [1, 2]}] +_NESTED_RENDERERS = [ + _to_claude_hook_entries, + _to_codex_hook_entries, + _to_gemini_hook_entries, + lambda entries: _to_antigravity_hook_entries(entries, "PreToolUse"), +] + + +def _nested_entries() -> list[dict[str, Any]]: + """Build independent source metadata with binding provenance.""" + return [ + { + "matcher": "Bash", + "_apm_source": "pkg", + "labels": {"team": ["core"]}, + "hooks": [ + { + "type": "command", + "command": "./scripts/check.sh", + "timeout": 5, + "env": copy.deepcopy(_ENV), + "args": copy.deepcopy(_ARGS), + } + ], + } + ] + + +@pytest.mark.parametrize( + ("render", "timeout"), + list(zip(_NESTED_RENDERERS, [5, 5, 5000, 5], strict=True)), + ids=["claude", "codex", "gemini", "antigravity-nested"], +) +def test_nested_targets_preserve_binding_metadata_and_provenance( + render: Callable[[list], list], timeout: int +) -> None: + """Nested native groups keep metadata and existing binding provenance.""" + entries = _nested_entries() + expected = copy.deepcopy(entries) + expected[0]["hooks"][0]["timeout"] = timeout + + assert json.loads(json.dumps(render(entries))) == expected + + +def test_antigravity_flat_preserves_metadata_and_provenance() -> None: + """Flat Antigravity output retains arrays and ownership without a wrapper.""" + rendered = _to_antigravity_hook_entries( + [{"_apm_source": "pkg", "bash": "echo test", "env": _ENV, "args": _ARGS, "timeout": 3}], + "SessionStart", + ) + + assert json.loads(json.dumps(rendered)) == [ + { + "env": _ENV, + "args": _ARGS, + "command": "echo test", + "timeout": 3, + "_apm_source": "pkg", + } + ] + + +@pytest.mark.parametrize( + "render", + [*_NESTED_RENDERERS, lambda entries: _to_antigravity_hook_entries(entries, "SessionStart")], + ids=["claude", "codex", "gemini", "antigravity-nested", "antigravity-flat"], +) +def test_raw_entries_are_materialized_independently(render: Callable[[list], list]) -> None: + """Each raw-entry adapter returns mutable JSON detached from source arrays.""" + raw = [{"env": {"APM_TEST": "value"}}, ["x", {"y": [1]}]] + source = copy.deepcopy(raw) + + rendered = render([raw]) + + assert json.loads(json.dumps(rendered)) == [source] + rendered[0][0]["env"]["APM_TEST"] = "mutated" + rendered[0][1][1]["y"].append(2) + assert raw == source + assert render([raw]) == [source] + + +def test_rendered_binding_handler_and_raw_output_are_independent_of_ir_and_source() -> None: + """Mutation of binding, handler and raw containers cannot reach source or IR.""" + entries = [*_nested_entries(), [{"env": {"A": ["1"]}}]] + source = copy.deepcopy(entries) + document = _entries_to_ir(entries) + + first = _render_nested_document(document, timeout_milliseconds=False) + first[0]["labels"]["team"].append("mutated") + first[0]["hooks"][0]["env"]["NESTED"]["LEVEL"][1]["b"] = 2 + first[0]["hooks"][0]["args"].clear() + first[1][0]["env"]["A"].append("mutated") + + assert entries == source + assert ( + json.loads(json.dumps(_render_nested_document(document, timeout_milliseconds=False))) + == source + )