GeezerVison is showing github.com — fix(hooks): type untyped flat command handlers for Codex by Parth-Vasave · Pull Request #3385 · microsoft/apm
Conversation
Description
A flat hook entry may omit type (Cursor defaults it to command). APM already wraps such entries in Codex's { "matcher", "hooks": [...] } groups, but the generated handler had no type. In the pinned Codex source (hook_config.rs at 36512a34), HookHandlerConfig is #[serde(tag = "type")] with no default, so an untyped handler matches no variant.
This is the Codex counterpart of #3130 / #3153. It reuses the renderer and ownership code merged there; there is no second implementation.
- Rendering: the flat-command type default from fix(hooks): type untyped flat command handlers for Claude #3153 (now
_with_default_command_type) also runs in _to_codex_hook_entries. Untyped flat command entries render as "type": "command" in .codex/hooks.json and its sidecar, including when they join groups from a Codex-shaped hook file in the same package.
- Unchanged: explicit types (
command, prompt, mcp_tool), untyped non-command entries, handlers already nested in the source, matchers, timeouts, and other targets (Cursor and Gemini are checked by a guard test).
- Upgrade path:
_legacy_hook_entries for Codex now returns both earlier shapes APM wrote: the original flat entry (before fix(hooks): render Codex hooks in native shape #3060) and the nested untyped group. Proven APM-owned output, including stale root-package sources healed by content match, therefore converges to one typed entry on reinstall and apm update. Hooks with no ownership record are never rewritten or removed, even when they are byte-identical to old APM output.
Before (from the issue):
{ "matcher": "Write", "hooks": [ { "command": "node \".codex/hooks/demo-pkg/.apm/hooks/scripts/gate.mjs\"", "timeout": 10 } ] }
After:
{ "matcher": "Write", "hooks": [ { "type": "command", "command": "node \".codex/hooks/demo-pkg/.apm/hooks/scripts/gate.mjs\"", "timeout": 10 } ] }
Evidence and its limits: this establishes that the generated output matches Codex's source schema. I have not observed a Codex runtime rejecting the old output or accepting the new output.
Issue and approved scope
Issue: #3249
Human scope-approval comment: #3249 (comment) (clarified in #3249 (comment))
This PR completes the approved scope. Out of scope, as agreed: defaults inside already-nested source handlers, camelCase event-name mapping, a shared default across all targets, guessing non-command types, and #2111.
Fixes #3249
Type of change
- Bug fix
- New feature
- Documentation
- Maintenance / refactor
Testing
- Tested locally
- All existing tests pass
- Added tests for new functionality (if applicable)
New and updated tests:
-
tests/unit/integration/test_hook_integrator_issue3249.py (14 cases):
- standalone and merged-with-Codex-shaped rendering;
- command-only defaulting, with explicit types preserved;
- nested source handlers unchanged;
- reinstall over owned flat and nested-untyped output;
- a manual untyped hook neither claimed nor removed;
- stale root-source healing for both legacy shapes.
9 of these fail without the fix. If only the new nested-untyped legacy matching is removed, the stale-root case fails.
-
test_required_codex_untyped_hook_upgrade_preserves_ownership_and_converges runs the installed CLI with {standalone, merged} × {nested-untyped, flat} legacy output. It covers first install, two reinstalls and apm update to a new revision, and checks that:
- a single typed entry results and the state converges;
- the user and manual hooks are kept;
- the
.apm sources are unchanged;
- an unrelated target's config is untouched;
- a sentinel shows the hook never ran during install.
All 4 variants fail without the fix.
-
test_codex_native_edge_types_untyped_flat_commands covers the renderer and the legacy shapes.
-
In the [BUG] Flat hook entries without type reach .claude/settings.json without the required type: command #3130 suite, the guard asserting Codex did not get the default now targets Cursor and Gemini.
Run locally, mirroring ci.yml:
- Lint: ruff check and format on CI's paths, the YAML/file-length/
relative_to checks, pylint R0801, and the auth-signal and architecture-boundary linters.
- Architecture ratchet contracts (188) and the scaling gate.
- Unit, console and red-team suites: 25,771 passed, 89.33% coverage.
- Lifecycle smoke, both shards: 412 passed.
- Audit workspace identity (6 passed) and
apm audit --ci.
Not run locally: the Windows jobs and the PyInstaller-built binary (I used the dev install's apm).
Docs: hooks-and-commands.md and the apm-guide package-authoring.md now describe the Claude and Codex input/output contract. The CHANGELOG has an entry under Unreleased.
Spec conformance (OpenAPM v0.1)
- N/A -- this PR does not change OpenAPM-observable behaviour.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Codex's HookHandlerConfig is an internally tagged enum
(#[serde(tag = "type")]) with no default, so the nested handler APM
generated from a flat command entry without `type` matched no variant.
Share the flat-command type default introduced for Claude in microsoft#3153 with
the Codex renderer: untyped flat command entries now render as
`"type": "command"` handlers in .codex/hooks.json, including when they
join existing groups. Explicit types, non-command entries and handlers
already nested in the source are unchanged, as are other targets.
Ownership matching on reinstall and update now recognises both earlier
Codex shapes APM wrote (the original flat entry and the nested untyped
group), so proven APM-owned output converges to one typed entry.
Manual and unowned hooks are not rewritten or removed.
Fixes microsoft#3249
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Thank you for contributing this Codex hook correction. Issue #3249 has a current maintainer-approved scope record for flat command handlers and owned legacy-output repair. The requested reviewers include Daniel Meppiel (@danielmeppiel) and Sergio Sisternes (@sergio-sisternes-epam). This is a recommendation for human review only, not merge approval.
Generated by autopilot-pr-triage-worker. This comment is AI-generated and may contain errors.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
APM Review Panel: ship_with_followups
Community fix types Codex flat command handlers, closing a gap where untyped entries rendered without the type discriminator Codex requires -- clean architecture, strong lifecycle tests, three foldable follow-ups.
panel-mode=lean; personas=python-architect,test-coverage-expert,doc-writer (adhoc: PR edits hook docs, apm-guide guidance and CHANGELOG),apm-ceo
cc parth (@Parth-Vasave) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.
All three panelists converge: the code change is narrow, correct, and aligned with the scoped decision on #3249 that limits this to input-shape defaulting only. The python-architect found zero correctness or abstraction risks -- the shared _with_default_command_type helper is minimal, respects explicit types, and the legacy-shape ownership matching reuses existing content-key discipline without introducing new ownership semantics. The 14 unit tests plus the contract-guard and installed-CLI lifecycle test form a strong regression net.
The one finding worth strategic attention is the missing denied-consent assertion for untyped flat Codex input. The consent gate (install/exec_gate.py) is upstream of rendering and untouched by this PR, and the existing hook-path lifecycle test already exercises the same gate with typed Codex input, so the regression risk is low. But the approved scope names "preserve denied-consent behavior", and without a test on the new input shape that promise holds only by coincidence. Fold it before merge.
The doc-writer findings are housekeeping: the CHANGELOG entry references the issue instead of the PR per repo convention (and the main merge left a duplicate Fixed heading), and the apm-guide omits the "supply type in nested groups" instruction that the hooks doc page already states.
Aligned with: Secure by default (consent gate upstream and untouched; the untyped-flat denial test is the one gap to fold); multi-harness (Codex now gets the same flat-command default Claude already has, without leaking target semantics into the neutral IR); community-driven (clean, well-tested fix on a maintainer-scoped issue); pragmatic as npm (authors no longer need type: command on flat Codex hook entries).
Panel summary
| Persona |
B |
R |
N |
Takeaway |
| Python Architect |
0 |
0 |
0 |
Shared default-type helper and legacy-shape ownership matching are correct, minimal, and respect single-canonical-owner discipline. |
| Test Coverage Expert |
0 |
1 |
0 |
Unit + contract-guard + lifecycle coverage is strong; denied consent is not exercised with untyped flat Codex hooks. |
| Doc Writer |
0 |
2 |
0 |
Hook doc matches the code; fix the CHANGELOG entry format and tighten the apm-guide wording. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- [Test Coverage Expert] Add a denied-consent assertion for untyped flat Codex hook input in installed-CLI lifecycle coverage. -- The approved scope names "preserve denied-consent behavior"; no test proves it for the new input shape.
- [Doc Writer] Fix the CHANGELOG entry: merge into the existing Fixed heading and end with
-- by @Parth-Vasave (#3385). -- Repo convention requires the PR number; the main merge also left a duplicate Fixed section under [Unreleased].
- [Doc Writer] Align the apm-guide package-authoring section with the hooks doc: state the shared flat-command default and that nested source groups must supply
type. -- The guide is the agent-facing authoring surface and currently omits the nested-group instruction.
Recommendation
Ship after the three recommended items are folded -- the denied-consent test gap, the CHANGELOG format fix, and the apm-guide alignment. All three are in-scope and bounded; the merge worker is folding them into this PR. The code itself is clean per architecture review and well covered by unit, contract and lifecycle tests. No blocking findings.
Full per-persona findings
Python Architect
No findings.
Test Coverage Expert
- [recommended] Denied-consent path not tested with untyped flat Codex hook input at
tests/integration/test_hook_path_package_lifecycle.py:315
The existing denied-consent block exercises allowExecutables={} for Codex with a typed nested descriptor only. The gate is upstream of rendering, so risk is low, but the approved scope lists it explicitly.
Suggested: Consumer with allowExecutables={}, install the untyped flat source, assert no .codex/hooks.json, no hook-executed sentinel, exec_status gated_pending_approval.
Proof (missing at): tests/integration/test_hook_path_package_lifecycle.py::test_hook_path_install_restore_update_remove -- proves: denied consent prevents hook deployment for untyped flat Codex hooks [secure-by-default]
Doc Writer
- [recommended] CHANGELOG entry ends with
(closes #3249) instead of (#3385) and adds a second ### Fixed heading under [Unreleased] at CHANGELOG.md:10
changelog.instructions.md requires -- by @username (#PR_NUMBER); one heading per type.
Suggested: Move the bullet into the existing Fixed list and end it with -- by @Parth-Vasave (#3385).
- [recommended] apm-guide repeats the type-default rule per target, names the nested scope inconsistently, and omits "supply type yourself in nested groups" at
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md:235
The code checks nesting in the source entry for both targets; the docs page already says so.
Suggested: One paragraph stating the shared flat-command default, per-target wrapping, and that nested source groups must supply type.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This branch has not been deployed
No deployments
Reviewers
At least 0 approving reviews are required to merge this pull request.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
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.
|