fix(hooks): type untyped flat command handlers for Claude - #3153
Draft
parth (Parth-Vasave) wants to merge 2 commits into
Draft
parth (Parth-Vasave) wants to merge 2 commits into
parth (Parth-Vasave) wants to merge 2 commits into
Conversation
Flat hook command entries that omit `type` (valid in Cursor, where it defaults to `command`) were wrapped into Claude's matcher groups without the handler `type` field that Claude's hook schema requires. The Claude renderer now supplies `"type": "command"` for flat entries that the neutral hook grammar reads as command handlers. Explicit handler types, untyped handlers already inside nested Claude groups, non-command entries, and all other targets are unchanged. Reinstall cleanup also matches the pre-fix untyped render, so stale root-package entries keep healing instead of duplicating (the microsoft#1329 / microsoft#1392 contract), mirroring the existing Codex legacy-content-key path. Refs microsoft#3130 apm-spec-waiver: Claude-native handler type default for microsoft#3130; OpenAPM v0.1 defines no target-native hook handler fields, the portable hook subset is deferred to microsoft#2111 (out of approved scope), and req-lk-021 ownership reconciliation is preserved.
Author
|
@microsoft-github-policy-service agree |
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
Claude-native rendering now supplies
"type": "command"for flat hook command entries that omittype, so.claude/settings.jsonreceives schema-valid handlers. Explicit handler types, nested Claude groups, non-command entries and every other target are unchanged.TL;DR
Flat hook entries without
typeare valid in Cursor (wheretypedefaults tocommand), but the Claude render wrapped them into{ "matcher", "hooks": [...] }groups and left the handler without thetypefield Claude's hook schema requires. The Claude renderer now adds it for recognized command entries only. A 6-line companion change keeps reinstall cleanup recognizing entries written before this fix.Note
Open question for the review contact (asked on the issue, still unanswered): should an untyped handler that is already inside a nested Claude group also get
type: "command"? The scope line says "recognized command handlers"; Done-when names flat input. This PR covers flat input only and pins that with a test, so extending it is a one-condition change if you prefer.Problem (WHY)
preToolUsewithcommand,matcher,timeout, notype) renders a Claude handler with notype. As the report puts it: "The nesting happens, but the handler is incomplete."PreToolUselist, still untyped..apm/hooksentries carrying a stale source marker (directory rename, worktree,apm.ymlname change) would be left behind as an untyped duplicate instead of healed, regressing the#1329/#1392contract.The approved Done-when requires that "Applicable untyped flat command input produces schema-valid Claude command handlers" and that ownership and other targets "remain unchanged". Per the maintainer's note, this PR claims schema validity only, not a demonstrated Claude Code runtime rejection.
Approach (WHAT)
type: "command"for a flat entry with notypethat the neutral hook grammar reads as a command (command, orbash/powershell/windowsnormalized tocommand).command,prompt, ...), nested Claude groups, and untyped entries without a command exactly as authored; no handler type is guessed.hook_contract.py,hook_ir.py) and the Codex, Gemini, Cursor and Antigravity renderers untouched, so no default reaches other targets.Implementation (HOW)
src/apm_cli/integration/hook_native_formats.py_with_claude_default_handler_type(), applied in_to_claude_hook_entries(), which gains a keyword-onlydefault_handler_type=Trueswitch. The command test reuses_handler_to_ir()from the neutral-grammar owner rather than defining "command handler" a second time.src/apm_cli/integration/hook_integrator.pylegacy_content_keysfrom_to_claude_hook_entries(entries, default_handler_type=False), like the Codex branch below it. Cleanup only recognizes one more content form; it removes nothingmainwould not have removed. File stays at 2,090 of 2,100 lines.tests/unit/integration/test_hook_integrator_issue3130.pycomponentmodule: 11 cases throughHookIntegratoron a temporary project (see Scenario Evidence).docs/src/content/docs/producer/author-primitives/hooks-and-commands.mdpackages/apm-guide/.apm/skills/apm-usage/package-authoring.mdCHANGELOG.mdFixedentry under[Unreleased].Trade-offs
#2111, which the approval excludes.Benefits
{"type": "command", "command": "...", "timeout": 10}; before, the handler had notype.settings.jsonand theapm-hooks.jsonsidecar.command/prompthandlers and non-command entries render exactly as before.Out-of-scope observations (not changed in this PR; happy to file issues)
apm.lock.yaml(fromtests/unit/install/test_install_target_copilot_app_e2e.py::TestCopilotAppParserE2E::test_project_scope_now_supported, nochdir),.github/mcp.json(fromTestRunMcpInstallSelfDefined::test_self_defined_dep_separated_correctly, duplicated in two files, no assertion), plus.codex/config.toml,apm_modules/,.local/state/gh/andbuild/lib/. CI's fresh checkouts hide this; locally it is easy to commit by accident.uv run pytest(documented as the full suite) skips every test:tests/perf/conftest.pyapplies its opt-in skip inpytest_collection_modifyitems, which receives all session items.v0.33.0release commit'suv.lockpoints about 100 packages atpackagefeedproxy.microsoft.ioinstead of PyPI.tests/test_*.pyandtests/fixtures/policy/test_fixtures_load.pycontain stale assertions that also fail onmain; CI's unit lanes do not run them.Issue and approved scope
Issue: Fixes #3130
Human scope-approval comment: #3130 (comment)
This PR completes the bounded issue scope: Claude-native typing of untyped flat command handlers (standalone and merged into an existing Claude-shaped group), preserved explicit and non-command types, regression coverage for both cases, and the related guidance. It does not change the shared hook representation, other targets, Codex event-name mapping, ownership or consent behavior.
Type of change
Testing
Validated at
cecf16beonmain18c4c43c(v0.33.0); the follow-up commit only adds this PR's number to the CHANGELOG entry. "All existing tests pass" is left unchecked because some local-only tests fail on this macOS host, identically onmain(listed below), and remote CI has not run yet.Validation evidence
Issue repro, Claude handler written to
.claude/settings.json, before (pre-fix renderer) and after:lint-auth-signals.sh,lint-architecture-boundaries.shtests/unit tests/test_console.py -n auto)test_claude_project_hook_runs_from_external_cwdneedspwsh, absent on this host; fails identically onmainapm audit --ci(APM Self-Check)npm ci,test:links,build, CLI docs contract, schema$ids)18c4c43c, which changes no docswindows_compatmarkerNew test run (verbatim)
Scenario Evidence
apm install --target claude, the handler in.claude/settings.jsonhastype: "command"(regression trap for #3130).tests/unit/integration/test_hook_integrator_issue3130.py::test_standalone_flat_untyped_entry_gets_command_type::test_flat_untyped_entry_merged_next_to_claude_shaped_filetypeexplicitly (command,prompt) or ship non-command entries see their handlers unchanged.::test_flat_handler_types_are_defaulted_only_for_commands(4 cases),::test_untyped_handler_inside_nested_claude_group_is_unchanged::test_claude_handler_default_does_not_reach_other_targets(2 cases)apm installover settings written by an older APM leaves one typed group, including stale root-package entries.::test_reinstall_replaces_untyped_group_written_before_fix,::test_stale_root_source_untyped_group_is_still_healedHow to test
uv run --frozen --extra dev pytest -q tests/unit/integration/test_hook_integrator_issue3130.py; expect 11 passed.preToolUseentry withouttype) and runapm install --target claude; thePreToolUsehandler in.claude/settings.jsonstarts with"type": "command"..claude/apm-hooks.jsonmirrors them.apm install --target codex,cursorwith the same package;.codex/hooks.jsonand.cursor/hooks.jsonmatchmain.Spec conformance (OpenAPM v0.1)
If this PR changes behaviour that an OpenAPM v0.1
req-XXXcovers,confirm the three-step ritual in the
development guide:
docs/src/content/docs/specs/openapm-v0.1.mdupdated(new/changed
<a id="req-XXX"></a>anchor + prose + Appendix Crow).
docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.ymlupdated.
@pytest.mark.req("req-XXX")test undertests/spec_conformance/added or extended.CONFORMANCE.{md,json}regenerated viauv run --extra dev python -m tests.spec_conformance.gen_statementand committed.
src/apm_cli/integration/is a Mode B critical path and this diff has exactly 20 substantive lines there. Noreq-XXXcovers target-native hook handler fields, and thereq-lk-021ownership reconciliation is preserved, not changed.apm-spec-waiver: Claude-native hook handler type default; OpenAPM v0.1 defines no target-native hook handler fields and req-lk-021 ownership reconciliation is preserved