diff --git a/.apm/architecture/owners/hooks-integrations.json b/.apm/architecture/owners/hooks-integrations.json index 588f42457..ff7ae4455 100644 --- a/.apm/architecture/owners/hooks-integrations.json +++ b/.apm/architecture/owners/hooks-integrations.json @@ -39,8 +39,8 @@ }, { "id": "neutral-hook-contract", - "decision": "Neutral hook source grammar, per-target native shape, and shared-config APM-owned drift projection", - "owner": "hook_contract.py (HOOK_COMMAND_KEYS, parse_hook_source, _entries_to_ir), per-target renderers, hook_ownership.py (project_apm_owned_hook_entries)", + "decision": "Neutral hook source grammar, per-target native shape, shared-config APM-owned drift projection, and ownership-sidecar comparison", + "owner": "hook_contract.py (HOOK_COMMAND_KEYS, parse_hook_source, _entries_to_ir), per-target renderers, hook_ownership.py (project_apm_owned_hook_entries, canonicalize_hook_sidecar)", "selectors": [ "src/apm_cli/hook_contract.py", "src/apm_cli/integration/hook_ir.py", diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c1a995f2..3dd2ed76c 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 + +- Hook ownership sidecars ignore JSON formatting during drift checks while preserving meaningful and invalid-content findings. — by @Pybsama (#3155) + ## [0.33.0] - 2026-10-02 ### Added diff --git a/docs/src/content/docs/enterprise/drift-detection.md b/docs/src/content/docs/enterprise/drift-detection.md index 21f6ce05f..04fd57725 100644 --- a/docs/src/content/docs/enterprise/drift-detection.md +++ b/docs/src/content/docs/enterprise/drift-detection.md @@ -48,9 +48,10 @@ only the deployment destination. `unrecorded` means replay produced the same normalized bytes as the project but no exact or directory `deployed_files` claim covers the path; shared merge-hook targets are exempt from `unrecorded`. For their drift comparison, only APM-owned hook entries are -considered; user-owned hooks do not create drift. The APM-owned sidecar remains -byte-for-byte checked, so tampered or missing APM-owned hooks report -`modified`. Pass `--no-drift` to skip the replay. +considered; user-owned hooks do not create drift. The APM-owned sidecar is +compared as JSON: object-key order and formatting are ignored, but all fields +and list order remain significant. Changed content, malformed JSON, and +duplicate keys report `modified`. Pass `--no-drift` to skip the replay. In bare `apm audit`, the replay remains cache-only and a fresh checkout without a warm cache yields an informational skip. In `apm audit --ci`, a cold cache instead triggers the lock-pinned scratch self-hydration owned by diff --git a/docs/src/content/docs/reference/baseline-checks.md b/docs/src/content/docs/reference/baseline-checks.md index ab6898df1..d629472ca 100644 --- a/docs/src/content/docs/reference/baseline-checks.md +++ b/docs/src/content/docs/reference/baseline-checks.md @@ -146,7 +146,7 @@ the [policy schema](../policy-schema/). ### `drift` - **What it verifies.** That the working tree matches what an install from the current lockfile would produce. The check replays the install pipeline into a scratch tree and diffs the result against the project. -- **Fails when.** Any deployed file differs from the replay output (hand-edits, missing integrations, orphaned files). For shared hook configuration files, the comparison covers only APM-owned hook entries; user-owned hooks are preserved and do not create drift. The APM-owned sidecar remains byte-for-byte checked, so tampered or missing APM-owned hooks still fail. In `apm audit --ci`, a cold cache no longer yields a green skip: if `apm_modules/` is absent, APM self-hydrates a lock-pinned scratch install first and then diffs against the checkout. If that scratch replay cannot be materialized, the drift check fails closed with the replay error. +- **Fails when.** Any deployed file differs from the replay output (hand-edits, missing integrations, orphaned files). For shared hook configuration files, the comparison covers only APM-owned hook entries; user-owned hooks are preserved and do not create drift. The complete APM-owned sidecar is compared as JSON, ignoring object-key order and formatting but preserving all fields and list order; changed content, malformed JSON, and duplicate keys still fail. In `apm audit --ci`, a cold cache no longer yields a green skip: if `apm_modules/` is absent, APM self-hydrates a lock-pinned scratch install first and then diffs against the checkout. If that scratch replay cannot be materialized, the drift check fails closed with the replay error. - **Skips when.** Only in bare `apm audit`, where the replay remains cache-only and a missing cache yields an informational skip advising the user to run `apm install` first. - **Skip with.** `apm audit --ci --no-drift` (reduces coverage; reserve for performance-constrained CI loops). - **Remediation.** Run `apm install` to restore the deployed state, or revert the hand-edit. For a cache-miss skip, the same `apm install` warms the cache and enables the check on the next run. diff --git a/docs/src/content/docs/reference/cli/audit.md b/docs/src/content/docs/reference/cli/audit.md index b688ac3aa..1da9f97c8 100644 --- a/docs/src/content/docs/reference/cli/audit.md +++ b/docs/src/content/docs/reference/cli/audit.md @@ -252,8 +252,10 @@ integrations, orphaned files, and `unrecorded` files. `unrecorded` applies when replay produced the same normalized bytes as the project but no exact or directory `deployed_files` claim covers the path. For shared merge-hook targets, audit compares only APM-owned hook entries; user-owned hooks do not -create drift. The APM-owned sidecar remains byte-for-byte checked, so tampered -or missing APM-owned hooks report `modified`. `unrecorded` findings fail +create drift. The APM-owned sidecar is compared as JSON, ignoring object-key +order and formatting while retaining every field and list order. Changed +commands, ownership markers, or execution order still report `modified`, as +do malformed JSON and duplicate keys. `unrecorded` findings fail `--ci`; run `apm install`, then commit the regenerated `apm.lock.yaml`. Drift is whole-project only; `--file` and explicit `PACKAGE` runs skip it. diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index f8055348c..10f845367 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -287,6 +287,12 @@ descendants, are skipped. ## Security and audit +For shared hook targets, drift compares only APM-owned hook entries. Ownership +sidecars are compared as JSON: object-key order and formatting do not cause +drift, but changed fields, ownership markers, or list order do. Malformed JSON, +duplicate keys, and non-JSON constants (`NaN`, `Infinity`, `-Infinity`) report +`modified`. User-owned hooks do not cause drift. + Automatic `apm audit` discovers registry-recognized primitive files, including untracked native hook definitions in shared settings. It checks prompt documents and decoded native prompt fields, not command strings, executables, diff --git a/scripts/architecture_linter/checks/mutation_hook_contract.py b/scripts/architecture_linter/checks/mutation_hook_contract.py index 808295e61..42ce3dbdf 100644 --- a/scripts/architecture_linter/checks/mutation_hook_contract.py +++ b/scripts/architecture_linter/checks/mutation_hook_contract.py @@ -628,40 +628,37 @@ def _nhc_ownership_markers(provider: FactsProvider, rule_id: str) -> tuple[Viola def _nhc_drift_projection(provider: FactsProvider, rule_id: str) -> tuple[Violation, ...]: - """Shared hook drift projection must route through ``hook_ownership.py``.""" + """Shared hook drift comparisons must route through ``hook_ownership.py``.""" facts_by_path, failures = _read_required(provider, rule_id, (_HOOK_OWNERSHIP, _INSTALL_DRIFT)) findings: list[Violation] = list(failures) - if not failures: - findings.extend( - _require( - _count_regex_lines( - facts_by_path[_HOOK_OWNERSHIP], r"^def project_apm_owned_hook_entries\(" + for helper in ("project_apm_owned_hook_entries", "canonicalize_hook_sidecar"): + if not failures: + findings.extend( + _require( + _count_regex_lines(facts_by_path[_HOOK_OWNERSHIP], rf"^def {helper}\(") == 1, + rule_id, + _HOOK_OWNERSHIP, + f"{helper} must be defined exactly once", + ) + ) + findings.extend( + _require( + _count_fixed_lines(facts_by_path[_INSTALL_DRIFT], f"{helper}(") == 2, + rule_id, + _INSTALL_DRIFT, + f"install drift must call {helper} exactly twice", ) - == 1, - rule_id, - _HOOK_OWNERSHIP, - "project_apm_owned_hook_entries must be defined exactly once", ) - ) findings.extend( - _require( - _count_fixed_lines(facts_by_path[_INSTALL_DRIFT], "project_apm_owned_hook_entries(") - == 2, - rule_id, - _INSTALL_DRIFT, - "install drift must call project_apm_owned_hook_entries exactly twice", + _duplicate_scan( + provider, + rule_id=rule_id, + paths=_python_paths(provider, under=_SRC, exclude=(_HOOK_OWNERSHIP,)), + pattern=rf"^def {helper}\(", + message="shared hook drift comparisons must route through hook_ownership.py", + exempt=False, ) ) - findings.extend( - _duplicate_scan( - provider, - rule_id=rule_id, - paths=_python_paths(provider, under=_SRC, exclude=(_HOOK_OWNERSHIP,)), - pattern=r"^def project_apm_owned_hook_entries\(", - message="shared hook drift projection must route through hook_ownership.py", - exempt=False, - ) - ) return tuple(findings) diff --git a/src/apm_cli/install/drift.py b/src/apm_cli/install/drift.py index 2ce8b0336..2a35e1a82 100644 --- a/src/apm_cli/install/drift.py +++ b/src/apm_cli/install/drift.py @@ -901,13 +901,13 @@ def diff_scratch_against_project( ) # Hook merge targets are shared with the user and never claimed in # deployed_files, so they can never be "unrecorded". Their APM-owned slice - # is compared through hook_ownership; sidecars remain byte-for-byte owned. + # is compared through hook_ownership; sidecars compare all owned content + # while ignoring JSON object-key ordering from the live merge. from apm_cli.install.manifest_reconcile import merge_hook_config_projection_specs merge_config_specs = merge_hook_config_projection_specs(targets) - merge_config_paths = set(merge_config_specs) | { - sidecar_path for sidecar_path, _ in merge_config_specs.values() - } + merge_sidecar_paths = {sidecar_path for sidecar_path, _ in merge_config_specs.values()} + merge_config_paths = set(merge_config_specs) | merge_sidecar_paths # Imperative local bundles have no authored source tree for replay. Their # deployed bytes are already bound by local_deployed_file_hashes and the @@ -976,6 +976,11 @@ def _is_canvas(rel: str) -> bool: _normalize((project_root / sidecar_rel).read_bytes()), event_container_key, ) + elif rel in merge_sidecar_paths: + from apm_cli.integration.hook_ownership import canonicalize_hook_sidecar + + s_bytes = canonicalize_hook_sidecar(s_bytes) + p_bytes = canonicalize_hook_sidecar(p_bytes) except (OSError, ValueError) as exc: findings.append( DriftFinding( diff --git a/src/apm_cli/install/manifest_reconcile.py b/src/apm_cli/install/manifest_reconcile.py index b1eefcbd8..5a618299c 100644 --- a/src/apm_cli/install/manifest_reconcile.py +++ b/src/apm_cli/install/manifest_reconcile.py @@ -208,8 +208,8 @@ def merge_hook_config_paths(targets: list[TargetProfile]) -> set[str]: tracking -- the same state ``reconcile_dropped_merge_hook_targets`` below exists to reconcile. A membership-driven check must therefore exempt them, or it reports every hooks-using project as under-recording. Content - coverage is unaffected: the drift replay reproduces these files and - compares them byte-for-byte. + coverage is unaffected: the drift replay compares APM-owned native hook + entries and the complete JSON content of their ownership sidecars. Lives here rather than beside ``_MERGE_HOOK_TARGETS`` because ``hook_integrator.py`` is at its CI line-count budget, the same reason diff --git a/src/apm_cli/integration/hook_ownership.py b/src/apm_cli/integration/hook_ownership.py index 4d7b4f146..a219b7944 100644 --- a/src/apm_cli/integration/hook_ownership.py +++ b/src/apm_cli/integration/hook_ownership.py @@ -62,6 +62,37 @@ def extract_apm_source_sidecar(hooks: dict) -> dict[str, list[dict[str, Any]]]: return sidecar +def _reject_duplicate_keys(pairs: list[tuple[str, Any]]) -> dict[str, Any]: + result: dict[str, Any] = {} + for key, value in pairs: + if key in result: + raise ValueError(f"duplicate JSON key: {key}") + result[key] = value + return result + + +def _reject_non_json_constant(constant: str) -> None: + raise ValueError(f"invalid JSON constant: {constant}") + + +def canonicalize_hook_sidecar(sidecar_bytes: bytes) -> bytes: + """Compare all ownership content without depending on JSON object-key order. + + Keep list order and every field, including ownership markers. Reject + duplicate keys and non-JSON constants rather than normalizing invalid input. + """ + # The integrator reads sidecars as UTF-8; json.loads(bytes) would also + # accept UTF-16/32 files that the next install cannot read. + sidecar = json.loads( + sidecar_bytes.decode("utf-8"), + object_pairs_hook=_reject_duplicate_keys, + parse_constant=_reject_non_json_constant, + ) + if not isinstance(sidecar, dict): + raise ValueError("ownership sidecar must be a JSON object") + return json.dumps(sidecar, sort_keys=True, separators=(",", ":")).encode("utf-8") + + def project_apm_owned_hook_entries( config_bytes: bytes, sidecar_bytes: bytes, @@ -75,14 +106,6 @@ def project_apm_owned_hook_entries( ``_apm_source`` markers; retain only that marked slice for comparison. """ - def _reject_duplicate_keys(pairs: list[tuple[str, Any]]) -> dict[str, Any]: - result: dict[str, Any] = {} - for key, value in pairs: - if key in result: - raise ValueError(f"duplicate JSON key: {key}") - result[key] = value - return result - config = json.loads(config_bytes, object_pairs_hook=_reject_duplicate_keys) sidecar = json.loads(sidecar_bytes, object_pairs_hook=_reject_duplicate_keys) if not isinstance(config, dict) or not isinstance(sidecar, dict): diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index dc1c320ae..07d1b0e94 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -55,6 +55,45 @@ OWNERS_DIR = ROOT / ".apm/architecture/owners" +@pytest.mark.parametrize( + ("path", "old", "new"), + [ + ( + "src/apm_cli/integration/hook_ownership.py", + "def canonicalize_hook_sidecar(", + "def canonicalize_hook_sidecar_disabled(", + ), + ( + "src/apm_cli/install/drift.py", + "s_bytes = canonicalize_hook_sidecar(s_bytes)", + "s_bytes = s_bytes", + ), + ( + "src/apm_cli/install/drift.py", + "p_bytes = canonicalize_hook_sidecar(p_bytes)", + "p_bytes = p_bytes", + ), + ( + "src/apm_cli/install/drift.py", + "def diff_scratch_against_project(", + "def canonicalize_hook_sidecar(data):\n return data\n\n\ndef diff_scratch_against_project(", + ), + ], +) +def test_hook_sidecar_comparison_must_use_canonical_owner(path: str, old: str, new: str) -> None: + """Both drift inputs must use the single sidecar comparison authority.""" + rule_id = "mutation_writes.neutral_hook_contract" + 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_id,), source_overrides={path: mutated}) + + assert report.failures == () + assert any(violation.rule_id == rule_id for violation in report.violations) + + @dataclass(frozen=True) class MutationCase: """One guard's minimal source mutation and the rule that must catch it. diff --git a/tests/integration/test_hook_sidecar_drift_e2e.py b/tests/integration/test_hook_sidecar_drift_e2e.py new file mode 100644 index 000000000..32f19b633 --- /dev/null +++ b/tests/integration/test_hook_sidecar_drift_e2e.py @@ -0,0 +1,66 @@ +"""Hermetic install/audit regression for ownership sidecar object-key order.""" + +from __future__ import annotations + +import json +import os +from pathlib import Path + +import pytest + +from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner +from tests.utils.isolated_apm_environment import IsolatedApmEnvironment +from tests.utils.lifecycle_state import LifecycleStateSnapshot +from tests.utils.local_package import LocalPackageFactory + +pytestmark = pytest.mark.integration + + +def test_preexisting_hook_event_order_does_not_cause_sidecar_drift( + tmp_path: Path, apm_engine_command: tuple[str, ...] +) -> None: + isolated = IsolatedApmEnvironment.create(tmp_path / "scenario", base_env=dict(os.environ)) + environment = isolated.subprocess_env() + factory = LocalPackageFactory(isolated.work_root) + project = factory.create("hook-order", targets=("claude",)) + factory.add_instruction(project, "rules", '---\napplyTo: "**"\n---\nBe brief.\n') + user_hook = {"hooks": [{"type": "command", "command": "echo user"}]} + settings_path = project.root / ".claude" / "settings.json" + settings_path.parent.mkdir() + settings_path.write_text(json.dumps({"hooks": {"SessionStart": [user_hook]}}), encoding="utf-8") + factory.add_hook( + project, + "events", + { + "hooks": { + "PreToolUse": [ + {"matcher": "Bash", "hooks": [{"type": "command", "command": "echo tool"}]} + ], + "SessionStart": [{"hooks": [{"type": "command", "command": "echo start"}]}], + } + }, + ) + runner = ApmLifecycleRunner(apm_engine_command) + install_args = ("install", "--no-policy", "--parallel-downloads", "0") + first = runner.run(install_args, cwd=project.root, env=environment) + assert first.returncode == 0, first.stdout + first.stderr + sidecar_path = project.root / ".claude" / "apm-hooks.json" + installed_bytes = sidecar_path.read_bytes() + assert list(json.loads(installed_bytes)) == ["SessionStart", "PreToolUse"] + + second = runner.run(install_args, cwd=project.root, env=environment) + assert second.returncode == 0, second.stdout + second.stderr + assert sidecar_path.read_bytes() == installed_bytes + before = LifecycleStateSnapshot.capture(project.root, targets=("claude",)) + + result = runner.run( + ("audit", "--ci", "--no-policy", "--format", "json"), + cwd=project.root, + env=environment, + ) + + assert result.returncode == 0, result.stdout + result.stderr + assert json.loads(result.stdout)["drift"]["drift"] == [] + assert LifecycleStateSnapshot.capture(project.root, targets=("claude",)) == before + settings = json.loads(settings_path.read_text(encoding="utf-8")) + assert user_hook in settings["hooks"]["SessionStart"] diff --git a/tests/unit/install/test_drift.py b/tests/unit/install/test_drift.py index 7971f4114..2223fbd43 100644 --- a/tests/unit/install/test_drift.py +++ b/tests/unit/install/test_drift.py @@ -387,6 +387,84 @@ def test_diff_engine_hook_merge_target_tampering_is_modified(tmp_path): assert [(finding.kind, finding.path) for finding in findings] == [("modified", rel)] +@pytest.mark.parametrize( + ("project_bytes", "modified"), + [ + (b'{"B":[], "A":[{"command":"one","timeout":1},{"command":"two"}]}', False), + (b'{"A":[{"timeout":1,"command":"one"},{"command":"two"}],"B":[]}', False), + (b'{"A":[{"command":"changed","timeout":1},{"command":"two"}],"B":[]}', True), + (b'{"A":[{"command":"one","timeout":true},{"command":"two"}],"B":[]}', True), + (b'{"A":[{"command":"two"},{"command":"one","timeout":1}],"B":[]}', True), + (b'{"A":[{"command":"one","timeout":1}],"B":[]}', True), + (b'{"A":[{"command":"one","timeout":1},{"command":"two"}],"B":[],"C":[]}', True), + (b'{"A":[],"A":[{"command":"one","timeout":1},{"command":"two"}],"B":[]}', True), + ( + b'{"A":[{"command":"discarded","command":"one","timeout":1},{"command":"two"}],"B":[]}', + True, + ), + (b"{invalid json", True), + (b"[]", True), + ('{"A":[{"command":"one","timeout":1},{"command":"two"}],"B":[]}'.encode("utf-16"), True), + ('{"A":[{"command":"one","timeout":1},{"command":"two"}],"B":[]}'.encode("utf-32"), True), + ], + ids=[ + "event-key-order", + "entry-key-order", + "command", + "value-type", + "execution-order", + "removed-entry", + "added-event", + "duplicate-event", + "duplicate-field", + "invalid-json", + "non-object", + "utf-16", + "utf-32", + ], +) +def test_diff_engine_hook_sidecar_compares_json_content(tmp_path, project_bytes, modified): + """Only object ordering/formatting is irrelevant; all content is owned.""" + from apm_cli.integration.targets import KNOWN_TARGETS + + scratch = tmp_path / "scratch" + project = tmp_path / "project" + rel = ".claude/apm-hooks.json" + _write(scratch / rel, b'{"A":[{"command":"one","timeout":1},{"command":"two"}],"B":[]}') + _write(project / rel, project_bytes) + + findings = diff_scratch_against_project( + scratch, project, _empty_lockfile(), targets=[KNOWN_TARGETS["claude"]] + ) + + assert [(finding.kind, finding.path) for finding in findings] == ( + [("modified", rel)] if modified else [] + ) + assert (project / rel).read_bytes() == project_bytes + + +@pytest.mark.parametrize("constant", ["NaN", "Infinity", "-Infinity"]) +def test_diff_engine_hook_sidecar_rejects_matching_non_json_constants(tmp_path, constant): + """Equal invalid JSON must not become clean drift through canonicalization.""" + from apm_cli.integration.targets import KNOWN_TARGETS + + scratch = tmp_path / "scratch" + project = tmp_path / "project" + rel = ".claude/apm-hooks.json" + invalid_bytes = ('{"PreToolUse":[{"timeout":' + constant + "}]}").encode() + _write(scratch / rel, invalid_bytes) + _write(project / rel, invalid_bytes) + + findings = diff_scratch_against_project( + scratch, project, _empty_lockfile(), targets=[KNOWN_TARGETS["claude"]] + ) + + assert [(finding.kind, finding.path) for finding in findings] == [("modified", rel)] + assert constant in findings[0].inline_diff + assert (scratch / rel).read_bytes() == invalid_bytes + assert (project / rel).read_bytes() == invalid_bytes + + def test_diff_engine_reports_malformed_shared_hook_event_as_modified(tmp_path): """A malformed native event remains drift rather than aborting audit.""" from apm_cli.integration.targets import KNOWN_TARGETS