fix(audit): ignore hook sidecar object-key order - #3155
Open
sama Pyb (Pybsama) wants to merge 3 commits into
Open
sama Pyb (Pybsama) wants to merge 3 commits into
sama Pyb (Pybsama) wants to merge 3 commits into
Conversation
sama Pyb (Pybsama)
requested review from
Daniel Meppiel (danielmeppiel) and
Sergio Sisternes (sergio-sisternes-epam)
as code owners
October 3, 2026 05:16
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Nonstandard JSON constants remain accepted, and required architecture and bundled-guide updates are missing.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Fixes false drift findings caused by JSON object-key ordering in hook ownership sidecars.
Changes:
- Canonicalizes sidecar JSON while preserving values and list order.
- Adds unit and end-to-end regression coverage.
- Updates audit documentation.
| File | Description |
|---|---|
src/apm_cli/integration/hook_ownership.py |
Adds sidecar canonicalization. |
src/apm_cli/install/drift.py |
Uses canonical comparison for sidecars. |
src/apm_cli/install/manifest_reconcile.py |
Updates reconciliation documentation. |
tests/unit/install/test_drift.py |
Covers sidecar comparison cases. |
tests/integration/test_hook_sidecar_drift_e2e.py |
Tests install and audit lifecycle. |
docs/src/content/docs/reference/cli/audit.md |
Documents JSON comparison behavior. |
docs/src/content/docs/reference/baseline-checks.md |
Updates drift-check semantics. |
docs/src/content/docs/enterprise/drift-detection.md |
Updates enterprise guidance. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return result | ||
|
|
||
|
|
||
| def canonicalize_hook_sidecar(sidecar_bytes: bytes) -> bytes: |
| """ | ||
| # 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) |
Comment on lines
+255
to
+258
| 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 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A clean install can fail
apm audit --ciwhen a consumer already has Claude hook events: the live merge and the empty scratch replay write ownership sidecar keys in different orders. Compare registered hook sidecars as UTF-8 JSON, preserving every field and list order while ignoring object-key order and formatting. Invalid encoding, malformed JSON, duplicate keys and non-JSON constants (NaN,Infinity,-Infinity) still produce drift findings. The existing ownership guard protects this comparison and its drift call sites; bundled CLI guidance matches the public documentation.Shared native hook configurations continue to compare only APM-owned entries. Lockfile hashing and ownership decisions are unchanged.
Issue and approved scope
Fixes #3062.
Human scope approval: #3062 (comment)
This covers the approved false-positive repair and preserves meaningful drift detection.
Type of change
Testing
The hermetic install/install/audit regression fails against the original source and passes with this change. It checks stable repeated installation, preservation of user hooks and read-only audit. Unit cases cover key reordering, changed commands/value types, entry removal, added events, execution order, duplicate keys, malformed JSON and unsupported UTF-16/32 encodings.
uv run pytest tests/unit/install/test_drift.py tests/integration/test_drift_check.py tests/integration/test_hook_sidecar_drift_e2e.py tests/unit/integration/test_hook_integrator.py tests/unit/integration/test_hook_integrator_defect_regression.py tests/unit/install/test_drift_phase3.py tests/integration/test_drift_check_e2e.py -q: 364 passed. Four architecture mutation regressions also pass; they cover missing comparison authority, duplicate ownership, and bypassed drift call sites.The six CI test-quality/architecture-ratchet modules also pass locally: 91 passed. Full-source Ruff lint/format and pylint duplication checks, YAML I/O/file-length/portable-path guards, auth and architecture boundary checks, and
git diff --checkpass. The full functional suite and Windows runtime were not run locally; these are local checks, not a claim that GitHub CI has run.Spec conformance (OpenAPM v0.1)
No normative requirement changes: sidecars are outside the deployed-file hash maps governed by req-lk-012/017, and this change does not alter the ownership/reconciliation obligations in req-lk-021.