Skip to content

fix(hooks): render nested hook metadata as plain JSON - #3211

Open
Abhishek Acharya (abhishekacharya200) wants to merge 9 commits into
microsoft:mainfrom
abhishekacharya200:fix-hook-metadata-json-rendering
Open

Abhishek Acharya (abhishekacharya200) wants to merge 9 commits into
microsoft:mainfrom
abhishekacharya200:fix-hook-metadata-json-rendering

Conversation

@abhishekacharya200

@abhishekacharya200 Abhishek Acharya (abhishekacharya200) commented Oct 12, 2026 •

Copy link
Copy Markdown

fix(hooks): preserve nested metadata in native JSON

Description

TL;DR

Warning

Conflict recovery is published, but readiness is blocked by an inherited main registry-matrix defect: test_matrix_covers_every_registry_guard_exactly_once lacks registry-delegation-build-id-verification (1 failed in 0.82s on the exact head). The matrix and architecture registry are unchanged from main. Under the release wind-down, no out-of-scope repair or further work will be started; this PR remains open.

Restore lossless native hook JSON during apm install and apm update without changing the immutable hook IR or supported adapter behavior. #3211 remains the sole implementation; selected, nonredundant regression tests from #3199 are incorporated with author credit.

Problem (WHY)

Approach (WHAT)

  • Keep one recursive _thaw beside _freeze in hook_contract.py.
  • Route handler, binding, nested raw-entry and flat raw-entry rendering through that owner.
  • Preserve mappings, arrays and scalars as independent native JSON values.
  • Keep existing matcher, command, timeout conversion and provenance behavior.
  • Adopt fix(hooks): serialize nested hook metadata in native hook files #3199's additional test scenarios, not its duplicate renderer-local helper.

Implementation (HOW)

File Change
src/apm_cli/hook_contract.py Own recursive mapping-to-dict and sequence-to-list materialization.
src/apm_cli/integration/hook_native_formats.py Use that owner at all four rendering boundaries.
scripts/architecture_linter/checks/mutation_hook_contract.py Register static protection against shallow metadata copies.
tests/integration/test_architecture_contract_guards.py Cover five rendering cases, Gemini compatibility, mutation isolation, static guards and four real native-file writes.
tests/unit/integration/test_hook_native_metadata.py Adopt binding/provenance, raw-array and source/IR isolation scenarios from #3199.
tests/integration/test_hook_nested_metadata_install.py Adapt #3199's CLI regression to a local package, normal approval, install/update and a no-execution sentinel.
docs/src/content/docs/producer/author-primitives/hooks-and-commands.md Document preserved nested metadata.
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md Keep packaged hook guidance consistent.
CHANGELOG.md Retain one fix entry and credit both contributors.

Diagrams

The dashed stage materializes independent native output; the source and immutable IR remain unchanged.

flowchart LR
    subgraph Source["Source"]
        S["Nested hook JSON"]
    end
    subgraph Contract["Canonical contract"]
        I["Immutable HookDocument"]
        T["hook_contract._thaw"]
    end
    subgraph Native["Existing native adapters"]
        O["Claude / Codex / Gemini / Antigravity JSON"]
    end
    S --> I
    I --> T
    T --> O
    classDef new stroke-dasharray: 5 5
    class T new
Loading

Trade-offs

Benefits

  1. Five rendering cases preserve nested JSON values without default=str.
  2. Mutating handler, binding or raw output cannot mutate source metadata or the frozen IR.
  3. Four actual native-file writes and a normal-policy CLI lifecycle cover the reported failure.

Issue and approved scope

Issue: #3167

Human scope-approval comment: #3167 (comment)

This PR completes that bounded scope. The maintainer subsequently selected #3211 as the sole landing vehicle, with useful #3199 tests retained and credited. No action closing or merging #3199 is part of this PR.

Fixes #3167

Note

Original Abhishek Acharya (@abhishekacharya200) commits are preserved. The selected tests from Mohammed Alkindi (@MohammedAlkindi)'s #3199 commit b36ed0ff2a60ee1974172d4d40bf21760ada7f7a are credited in commit c26ea4976b8591673e2067a92f525d0894c28beb using the original commit's author identity.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass
  • Added tests for new functionality (if applicable)

The full local repository suite was not run. The focused functional suite and all local lint gates below were run on exact head 2b794ed5f7a80ef834cc925e20d2ba01b4b440ad, including main through b71cb47cd99e665d285a497ba6d05e810ae42ea4.

Validation

uv run --frozen --extra dev pytest tests/integration/test_architecture_contract_guards.py tests/unit/integration/test_hook_native_metadata.py tests/integration/test_hook_nested_metadata_install.py -q --basetemp=../pr-3211-artifacts/consolidation/conflict-b71/pytest-exact --junitxml=../pr-3211-artifacts/consolidation/conflict-b71/tests-exact.xml:

46 passed in 59.22s

Historical mutation proof on bc941e0a4cd55f2b610107fede64d29fb17012bd (not rerun for this faithful merge): the same files with -k 'metadata or shallow or raw_entries or independent or nested_env', after replacing _thaw with identity and unregistering _nhc_native_metadata_thaw:

28 failed, 1 passed, 17 deselected in 28.58s

All 28 intended traps failed; the control passed. Both guards were restored byte-for-byte. The recovery changed neither these regression-trap files nor the thaw/static guard; the fresh 46-case run above passed on the new head. Historical mutation execution is not presented as a new run.

Exact local lint and consumer-path evidence
Command Result
uv run --frozen --extra dev ruff check src/ tests/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/ scripts/windows_native_symlink_probe_entry.py Passed
uv run --frozen --extra dev ruff format --check src/ tests/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/ scripts/windows_native_symlink_probe_entry.py Passed; 1960 files already formatted
uv run --frozen --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/ scripts/windows_native_symlink_probe_entry.py Passed
bash scripts/lint-auth-signals.sh Passed
bash scripts/lint-architecture-boundaries.sh Passed
uv run --frozen python scripts/check_test_assertions.py Passed
uv run --frozen python scripts/check_exact_test_duplicates.py Passed

The CI-equivalent YAML I/O, 2100-line and portable-relative-path guards also passed, using equivalent Python regex/file scans on macOS.

The installed .venv/bin/apm console script independently passed install with no hook approval (no native hooks emitted), approve ../hookpkg, approved install, and update --yes with fixture lock/config regeneration. Generated Claude/Codex metadata and original source bytes were checked; the hook-execution sentinel remained absent. These are real subprocesses, separate from the in-process CliRunner regression.

Scenario Evidence

# Scenario (user promise) Principle(s) Test(s) proving it Type
1 Install/update hooks with nested metadata without crashing or executing hooks DevX, Secure by default, Governed by policy tests/integration/test_hook_nested_metadata_install.py::test_install_update_write_nested_hook_metadata (regression-trap for #3167) integration
2 Keep metadata, matcher, command and timeout across native targets Multi-harness support tests/integration/test_architecture_contract_guards.py::test_integrator_writes_nested_hook_metadata_to_native_json integration
3 Preserve binding provenance and raw nested arrays Multi-harness support tests/unit/integration/test_hook_native_metadata.py unit
4 Editing generated metadata never changes source or immutable IR Secure by default tests/unit/integration/test_hook_native_metadata.py::test_rendered_binding_handler_and_raw_output_are_independent_of_ir_and_source unit

Conflict recovery

Faithful merge 2b794ed5f7a80ef834cc925e20d2ba01b4b440ad has parents bc941e0a4cd55f2b610107fede64d29fb17012bd and b71cb47cd99e665d285a497ba6d05e810ae42ea4. The only conflict was CHANGELOG.md; both #3211 and #3155 entries remain. All nine PR files have identical added/removed lines relative to the new base as the previously reviewed consolidation. Contributor commits, canonical thaw, consent checks and tests are preserved. The prior genuine delta/CEO review is carried forward; no new panel was run and no unlanded #3153 work was imported.

Technical CI

Observed standard technical workflows for exact head 2b794ed5f7a80ef834cc925e20d2ba01b4b440ad and tested base b71cb47cd99e665d285a497ba6d05e810ae42ea4:

Workflow Observed result
CodeQL completed / success
Merge Gate completed / success
NOTICE Drift Check completed / success
CI completed / success
Deploy Docs completed / success
Spec conformance completed / success

Docs build passed; deployment was skipped for the PR. Owner evidence version 2 includes fresh deterministic detection and five exact-head consumer test IDs. The blocked completion passes schema validation; the verifier's blocked-return shortcut is not presented as ready-state owner certification. The prior run's checks and preview are not used as current-head execution evidence.

The PR remains open and non-draft under the release wind-down. No merge, queue, auto-merge, reviewer request or readiness automation was enabled. Technical readiness does not grant platform admission or a release commitment.

Additional required gate: uv run --frozen --extra dev pytest tests/integration/test_architecture_owner_rule_mutations.py::test_matrix_covers_every_registry_guard_exactly_once -q --basetemp=../pr-3211-artifacts/consolidation/conflict-b71/pytest-matrix --junitxml=../pr-3211-artifacts/consolidation/conflict-b71/registry-matrix.xml failed: 1 failed in 0.82s, missing registry-delegation-build-id-verification. This blocks readiness regardless of narrower passing checks. No baseline fix was attempted.

How to test

  • Run uv run --frozen --extra dev pytest tests/integration/test_architecture_contract_guards.py tests/unit/integration/test_hook_native_metadata.py tests/integration/test_hook_nested_metadata_install.py -q; expect 46 passing cases.
  • Inspect the CLI regression: approval must precede hook output, update must regenerate the removed native configs, and the sentinel must remain absent.
  • Run bash scripts/lint-architecture-boundaries.sh; expect exit 0.

Spec conformance (OpenAPM v0.1)

If this PR changes behaviour that an OpenAPM v0.1 req-XXX covers,
confirm the three-step ritual in the
development guide:

  • Spec edit: docs/src/content/docs/specs/openapm-v0.1.md updated
    (new/changed <a id="req-XXX"></a> anchor + prose + Appendix C
    row).
  • Manifest edit: docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.yml
    updated.
  • Test edit: a @pytest.mark.req("req-XXX") test under
    tests/spec_conformance/ added or extended.
  • CONFORMANCE.{md,json} regenerated via
    uv run --extra dev python -m tests.spec_conformance.gen_statement
    and committed.
  • N/A -- this PR does not change OpenAPM-observable behaviour.

This restores existing supported metadata serialization; it introduces no normative schema or requirement.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The canonical thawing path lacks its required static architecture guard, and the changelog entry omits the PR reference.

2 open findings
What changed in this PR

Fixes nested hook metadata serialization while preserving immutable hook IR.

Changes:

  • Adds recursive metadata thawing at native render boundaries.
  • Adds regression coverage across supported renderers.
  • Updates documentation and changelog.
File Description
src/​apm_cli/​hook_contract.py Adds recursive metadata thawing.
src/​apm_cli/​integration/​hook_native_formats.py Thaws metadata before native rendering.
tests/​integration/​test_architecture_contract_guards.py Tests serialization and IR independence.
docs/​src/​content/​docs/​producer/​author-primitives/​hooks-and-commands.md Documents nested metadata preservation.
CHANGELOG.md Records the bug fix.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/apm_cli/hook_contract.py
Comment thread CHANGELOG.md Outdated
@abhishekacharya200

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

hook_contract freezes hook metadata recursively, but the native renderers only copied the top level. Nested mappings such as env stayed MappingProxyType and json.dumps failed during install and update.

Add _thaw next to _freeze and use it for handler metadata, binding metadata and raw entries in the nested and flat renderers. The IR stays frozen; rendered output is an independent copy.

Fixes microsoft#3167
Extend the neutral-hook-contract architecture rule so native hook renderers must thaw frozen metadata through hook_contract._thaw, with mutation tests that restore each shallow dict(...)/raw_entry copy and expect the rule to fail. Add the PR reference to the changelog entry.
Preserve upstream README link corrections and cleanup diagnostic changes before validating microsoft#3211.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a
Address the panel coverage follow-up for microsoft#3211 and accepted scope microsoft#3167. Exercise real HookIntegrator writes across Claude, Codex, Gemini and Antigravity, preserving metadata, matcher, command and timeout units. Keep packaged hook guidance aligned.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a
@danielmeppiel

Daniel Meppiel (danielmeppiel) commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

APM Review Panel: prior ship_now

Warning

Conflict resolved, but current readiness is blocked. The exact-head registry mutation-matrix gate failed because main registers registry-delegation-build-id-verification without a matrix case. Under release wind-down, this inherited, out-of-scope defect will not be repaired here.

#3211 remains the sole #3167 vehicle, with the selected #3199 tests and both contributors credited.

Review carried forward: the genuine test-coverage expert and CEO previously returned zero findings and ship_now. No new panel or persona was run for this mechanical merge. All nine PR files retain the same added/removed lines against the new base; the canonical thaw, native adapters and regression traps are unchanged.

Conflict and provenance

Refreshed exact-head evidence

  • Functional/static: 46 passed in 59.22s across the native contract, selected renderer metadata tests and local CLI lifecycle.
  • Real installed CLI: unapproved install, normal apm approve, approved install and regenerated update --yes all passed. Nested native JSON and source bytes were checked; the hook-execution sentinel remained absent. No --no-policy or --force.
  • Lint: all seven current gates, architecture boundary script, assertion-quality and exact-duplicate ratchets passed.
  • Required registry matrix: test_matrix_covers_every_registry_guard_exactly_once failed (1 failed in 0.82s) with missing registry-delegation-build-id-verification. The matrix and .apm/architecture tree are byte-identical to main. No repair or repeat investigation was attempted.
  • Owner evidence v2: fresh deterministic owner detection and five executed exact-head consumer test IDs are retained. The blocked completion is schema-valid; the verifier reports that ready-state evidence is not required for a blocked return, which is not a readiness certification.
  • Mutation provenance: the prior actual bc941e0a4cd55f2b610107fede64d29fb17012bd run produced 28 intended failures and one passing control. The trap files and protected thaw/static guard are unchanged; no new mutation execution is claimed. The fresh passing run above verifies them on the merged head.
  • Docs: prior resolved Starlight/packaged-guide work is carried forward with an identical PR documentation delta; no new docs work is needed.
Current-head technical workflows
Workflow Observed result
CodeQL completed / success
Merge Gate completed / success
NOTICE Drift Check completed / success
CI completed / success
Deploy Docs completed / success
Spec conformance completed / success

Docs deployment was skipped for this PR; its build passed. Earlier-head CI is retained as history, not substituted for these observed runs. Pending jobs are not treated as passed.

Disposition

The prior ship_now code/test recommendation is retained as history, not a claim that the current PR is ready. The required matrix failure blocks readiness despite the narrower passing evidence. Existing reviewer requests are unchanged; no reviewer was requested or review approved. #3199 remains untouched and open.

Per the release wind-down, #3211 is left open and non-draft. No merge, queue, auto-merge, new PR, readiness automation or protection change was performed. Platform admission remains separate from technical readiness. This recovery ends here without further optional work or a release commitment.


Generated by autopilot-pr-merge-worker. This comment is AI-generated and may contain errors.

Preserve upstream fixes and both existing release-note sections without rewriting the contributor history.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a
Select nonredundant regression assets from microsoft#3199 commit b36ed0f for the approved microsoft#3167 scope. Keep microsoft#3211 as the sole implementation and its canonical _thaw unchanged. Prove binding/raw/source independence and normal approved local install/update without executing hooks.

Co-authored-by: Mohammed Alkindi <alkndymhmd692@gmail.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a
Faithfully preserve main through f8e59dc without changing the reviewed hook implementation or tests.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a
Preserve current main through microsoft#3190 without altering the reviewed canonical hook rendering or consolidated regression assets.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a
Resolve only overlapping CHANGELOG additions by retaining both entries. Preserve the existing hook JSON repair, contributor history and regression coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f79e464c-180e-4be3-9056-0773aa679a1a

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] APM 0.33.0: apm update fails to integrate Azure Skills because frozen hook metadata is not JSON serializable

3 participants