fix(hooks): normalize flat entries for Claude#2097
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes Claude hook integration by normalizing flat (Copilot-style) hook entries into Claude’s nested matcher/hooks grouping prior to merge, keeping both .claude/settings.json and the APM ownership sidecar structurally aligned (and preventing Claude from skipping the whole settings file).
Changes:
- Extend the shared “wrap flat entries into nested groups” helper to optionally add a default matcher, and apply it for the
claudetarget. - Add a regression test covering settings + sidecar output shape for Claude (#2062).
- Document the “flat input -> nested Claude output” contract in Starlight docs and the bundled APM guide.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/integration/hook_integrator.py |
Adds a Claude-specific normalization step and extends the shared nesting helper with default_matcher. |
tests/unit/integration/test_hook_integrator_issue2062.py |
New regression test ensuring Claude output is nested (and sidecar preserves ownership). |
docs/src/content/docs/producer/author-primitives/hooks-and-commands.md |
Documents portable flat hook entries and shows Claude’s normalized nested output. |
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md |
Updates the packaged guide to describe the same flat->nested portability contract. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Low
| Shared by the Gemini and Antigravity transforms (both use the Claude | ||
| nested matcher shape for tool events). *key_fixer* renames the inner | ||
| command/timeout keys in place for the specific target. Entries already | ||
| in nested form have only their inner keys fixed. | ||
| command/timeout keys in place for the specific target. *default_matcher* | ||
| is added to newly wrapped entries when the target requires one. Entries | ||
| already in nested form have only their inner keys fixed. |
| `postToolUse`) shapes; events are renamed per target during merge. Flat | ||
| Copilot command entries are also portable: when the target is Claude, APM | ||
| wraps each one in Claude's required `matcher` and `hooks` group. |
| When deploying to Claude, APM wraps each flat entry in the required | ||
| `{"matcher": "*", "hooks": [...]}` group. Copilot keeps the flat entry, so | ||
| one source manifest is valid for both targets. Already nested Claude entries | ||
| remain nested. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | Clean shared-helper design; update one stale docstring. |
| CLI Logging Expert | 0 | 0 | 2 | No CLI output regression. |
| DevX UX Expert | 0 | 0 | 2 | Portable authoring contract is clear; explain the wildcard. |
| Supply Chain Security Expert | 0 | 0 | 1 | Global wildcard preserves flat-entry semantics. |
| OSS Growth Hacker | 0 | 0 | 2 | Lead release notes with write-once portability. |
| Doc Writer | 0 | 1 | 1 | Docs are accurate; keep mirrored guidance concise. |
| Test Coverage Expert | 0 | 2 | 1 | CLI test passes; assert sidecar and marker contract. |
| Performance Expert | 0 | 0 | 2 | O(entries) transform has negligible cost. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Assert the sidecar structure and use module-level
pytestmarkin the CLI integration test. - [Doc Writer] Keep the Starlight and bundled APM guide explanations concise and synchronized.
- [OSS Growth Hacker] Add a user-benefit CHANGELOG line and scannable source/output captions.
- [Supply Chain Security Expert] Explain that
matcher: "*"preserves the global semantics of a flat entry. - [Performance Expert] Replace the per-call no-op lambda with a named helper for clarity.
Recommendation
Fold the CLI-level sidecar proof, mutation-break evidence, and small code/doc polish, then run a final panel pass.
Full per-persona findings
- Python Architect: update the shared-helper docstring to include Claude; commit the new CLI integration test.
- CLI Logging Expert: make the Claude wrapper docstring precise; optional transform debug breadcrumb.
- DevX UX Expert: explain
matcher: "*"in both documentation surfaces. - Supply Chain Security Expert: state that the wildcard is semantically equivalent to flat global hooks.
- OSS Growth Hacker: add benefit-led release notes and source/output captions.
- Auth Expert: inactive; no auth surface changed.
- Doc Writer: keep duplicate mirrored prose bounded while preserving required APM guide synchronization.
- Test Coverage Expert: assert
.claude/apm-hooks.json, use module-level marker placement, and preserve nested-input behavior. - Performance Expert: name the no-op key fixer; shallow copies are expected and safe.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Add full CLI install coverage for settings and sidecar preservation, and fold the panel's documentation and regression-test follow-ups for #2062. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Clarify the flat-entry shape and limit the example claim to the Copilot and Claude targets it demonstrates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Clean reuse of the shared target-normalization strategy. |
| CLI Logging Expert | 0 | 0 | 0 | No CLI output or logging surface changed. |
| DevX UX Expert | 0 | 0 | 0 | The portable source and generated Claude shape are clear. |
| Supply Chain Security Expert | 0 | 0 | 0 | Provenance and global matcher semantics are preserved. |
| OSS Growth Hacker | 0 | 0 | 0 | Benefit-led release and docs framing is ready. |
| Doc Writer | 0 | 0 | 0 | Starlight and the bundled guide are accurate and synchronized. |
| Test Coverage Expert | 0 | 0 | 0 | CLI, unit, and mutation-break evidence protect the user promise. |
| Performance Expert | 0 | 0 | 0 | The O(entries) in-memory transform is negligible. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Folded in this run
- (panel) Added an empirical CLI integration test for
apm install --target claude-- resolved inc3650318c1118a401300e472f7afca84a5809a57. - (panel) Proved existing settings and valid nested hooks survive the install -- resolved in
c3650318c1118a401300e472f7afca84a5809a57. - (panel) Asserted normalized sidecar ownership and module-level integration marker placement -- resolved in
c3650318c1118a401300e472f7afca84a5809a57. - (panel) Clarified wildcard semantics, source/output captions, and the Copilot-and-Claude scope -- resolved in
c4e290220d523ae54f055d6317cf3c9f6b97ce13. - (panel) Added a benefit-led changelog entry and precise named Claude key strategy -- resolved in
c3650318c1118a401300e472f7afca84a5809a57.
Copilot signals reviewed
Copilot review round 1 produced a summary and zero inline findings. Round 2 after the first push also produced zero inline findings.
Regression-trap evidence (mutation-break gate)
tests/integration/test_claude_flat_hook_install_e2e.py::test_install_claude_nests_flat_hooks_and_preserves_settings-- deleted the Claude dispatch to_to_claude_hook_entries; the test failed on the unchanged flat entry; the guard was restored and the test passed.
Lint contract
The canonical ruff pair was silent at the final SHA. Pylint R0801 rated 10.00/10, the YAML/file-length/relative-path guards were clean, and scripts/lint-auth-signals.sh reported clean.
CI
All checks on exact head c4e290220d523ae54f055d6317cf3c9f6b97ce13 completed successfully or were intentionally skipped. CI run: https://github.com/microsoft/apm/actions/runs/29061945387
Mergeability status
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2097 | c4e2902 |
ship_now | 2 | 5 | 0 | 2 | green | MERGEABLE | BLOCKED | awaiting required review |
Recommendation
All in-scope findings are folded, exact-SHA CI is green, and the regression test is mutation-proven. Ready for maintainer review.
Full per-persona findings
Python Architect
- [nit] An optional
Nonekey-fixer could replace the named no-op helper. The named helper was explicitly requested in the prior pass and keeps the strategy callable and intent grep-able, so no change is recommended.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
PR touches hooks, tests, documentation, and changelog; no authentication surface changed.
Doc Writer
No findings.
Test Coverage Expert
No findings. Relevant tests passed, and the CLI regression test failed when the production guard was removed.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Fold panel doc nits: call them hook entries (not command entries) to disambiguate from slash-commands on the same page, and state explicitly that Copilot consumes the flat entry as-is while only Claude requires the nested matcher group. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e0c0aa7f-c584-4f8c-8931-960e4836d376
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python architect | 0 | 0 | 1 | Fix reuses the shared _to_nested_hook_entries helper with a no-op Claude key fixer; _apm_source provenance moves to the outer group for idempotent sidecar upsert. Sound and consistent with the gemini/antigravity siblings. |
| Cli logging expert | 0 | 0 | 1 | Value-safe: the transform adds no new user-facing output and leaks no command/env/secret payloads. Parity with the silent gemini/antigravity transforms is maintained. |
| Devx ux expert | 0 | 1 | 2 | Removes a silent-failure cliff (Claude previously rejected flat hooks and disabled them all); portable manifests now just work. Minor doc-wording polish suggested (now folded). |
| Supply chain security expert | 0 | 0 | 0 | No new trust boundary: a pure in-memory reshape of already-trusted local data, no network/exec surface, value-safe handling preserved. Sidecar upsert is idempotent. No findings. |
| Oss growth hacker | 0 | 0 | 1 | Fixes a real adoption cliff (portable hook manifests silently broke on Claude). CHANGELOG line is adequate; only optional wording polish. |
| Doc writer | 0 | 0 | 2 | Docs accurately describe the new flat->nested Claude behavior on both the Starlight page and the apm-guide usage resource. Starlight clarity clause now folded. |
| Test coverage expert | 0 | 0 | 0 | All critical hook/install promises defended by the e2e subprocess test and unit tests; mutation-break gate confirms the tests are load-bearing. Ship. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 1 follow-ups
- [Cli logging expert] Add a
_log.debugbreadcrumb when_to_nested_hook_entrieswraps flat entries, uniformly across Claude/Gemini/Antigravity targets. -- Aids field diagnosis when hooks silently do nothing; scoped out of this PR to avoid cross-target helper churn on a minimal-fix branch.
Recommendation
Merge once required checks are green on 308e01b. The fix is minimal, load-bearing tests prove the user promise holds, all in-scope nits are folded, and the single deferred observability item has a clean scope-boundary note. Track the debug-breadcrumb follow-up as a small DevX issue for a future uniform pass across all hook transforms.
Full per-persona findings
Python architect
- [nit] _preserve_claude_hook_keys is a no-op key fixer at
src/apm_cli/integration/hook_integrator.py
At a single call-site the named no-op is self-documenting; a shared sentinel would only pay off with a second Claude-family caller. No change needed now.
Cli logging expert
- [nit] Optional debug breadcrumb when wrapping flat entries at
src/apm_cli/integration/hook_integrator.py
A single_log.debugcounting wrapped entries would aid field diagnosis, but is not required; current silence matches sibling transforms.
Devx ux expert
- [recommended] Consider a debug breadcrumb on flat->nested wrap at
src/apm_cli/integration/hook_integrator.py
When a hook silently does nothing, authors have little to go on; a debug line would shorten diagnosis. Non-gating; parity with existing transforms is acceptable. - [nit] Disambiguate 'command entries' from slash-commands at
docs/src/content/docs/producer/author-primitives/hooks-and-commands.md
On a page titled hooks-and-commands, 'Copilot command entries' can be misread as slash-commands; 'hook entries' is clearer. (Folded at 308e01b.) - [nit] State that Copilot keeps the flat entry at
docs/src/content/docs/producer/author-primitives/hooks-and-commands.md
The portability section shows Claude output but did not explicitly say Copilot consumes the flat entry unchanged. (Folded at 308e01b.)
Supply chain security expert
No findings.
Oss growth hacker
- [nit] CHANGELOG could lead with user benefit over mechanism at
CHANGELOG.md
Optional stylistic preference; the precise term 'flat hook manifests' is accurate and matches the issue, so keeping it is defensible.
Auth expert -- inactive
PR #2097 touches only hook normalization (hook_integrator.py), tests, and docs. No token management, credential resolution, git auth, or remote-host surface is modified; the auth lens is not engaged.
Doc writer
- [nit] Make the Copilot-keeps-flat behavior explicit on the Starlight page at
docs/src/content/docs/producer/author-primitives/hooks-and-commands.md
The page showed Claude output but should state Copilot consumes the flat entry unchanged. (Folded at 308e01b.) - [nit] CHANGELOG [Unreleased] header has trailing whitespace at
CHANGELOG.md
Cosmetic markdownlint MD009; pre-existing on main and outside this PR's diff, so not introduced here.
Performance expert -- inactive
The transform is O(entries) over a handful of hook entries at install time, off any hot path (no resolution/download/materialization loop). Negligible cost; performance lens not engaged.
Test coverage expert
No findings.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
fix(hooks): normalize flat entries for Claude
TL;DR
Claude integration now converts flat Copilot-style command entries into Claude's required
matcher/hooksgroups before merge. The same normalized shape is written to.claude/settings.jsonand the APM ownership sidecar, preventing Claude from rejecting the entire settings file.Important
Closes #2062.
Problem (WHY)
hooks: Expected array, but received undefinedand skipped the settings file.The fix keeps the change deterministic and testable: "Grounding outputs in deterministic tool execution transforms probabilistic generation into verifiable action."
Approach (WHAT)
matcher: "*"; preserve already nested entries.Implementation (HOW)
src/apm_cli/integration/hook_integrator.pytests/unit/integration/test_hook_integrator_issue2062.pydocs/src/content/docs/producer/author-primitives/hooks-and-commands.mdpackages/apm-guide/.apm/skills/apm-usage/package-authoring.mdDiagrams
Legend: the dashed stage is the new target-specific normalization added before the existing ownership and write path.
flowchart LR subgraph Parse[Parse] P1[Flat hook entry] end subgraph Normalize[Normalize for target] N1[Claude matcher group] end subgraph Persist[Persist] W1[settings.json] W2[apm-hooks.json] end P1 --> N1 N1 --> W1 N1 --> W2 classDef new stroke-dasharray: 5 5; class N1 new;Trade-offs
matcher: "*"only for newly wrapped entries. Existing nested Claude entries retain their authored matcher rather than being rewritten.Benefits
PreToolUsecommand becomes one schema-valid Claude matcher group.Validation
uv run --extra dev pytest tests/unit/integration/test_hook_integrator_issue2062.py tests/unit/integration/test_hook_integrator.py tests/unit/integration/test_hook_naked_format.py -q:uv run --extra dev pytest tests/unit tests/test_console.py -q:Lint mirror and mutation evidence
uv run --extra dev ruff check src/ tests/:uv run --extra dev ruff format --check src/ tests/:uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/:bash scripts/lint-auth-signals.sh:Disabling the Claude transform made
test_hook_integrator_issue2062.pyfail on the unchanged flat entry; restoring it returned the test to green.Scenario Evidence
tests/unit/integration/test_hook_integrator_issue2062.py::test_claude_wraps_flat_hook_entries_in_settings_and_sidecar(regression-trap for #2062)How to test
1 passed.PreToolUsecommand with targetclaude..claude/settings.jsonand confirm the entry hasmatcher: "*"plus ahooksarray..claude/apm-hooks.jsonand confirm the same outer shape includes_apm_source.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com