Skip to content

fix(hooks): type untyped flat command handlers for Codex - #3385

Open
parth (Parth-Vasave) wants to merge 1 commit into
microsoft:mainfrom
Parth-Vasave:fix/3249-codex-hook-handler-type
Open

parth (Parth-Vasave) wants to merge 1 commit into
microsoft:mainfrom
Parth-Vasave:fix/3249-codex-hook-handler-type

Conversation

@Parth-Vasave

Copy link
Copy Markdown
Contributor

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.

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

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

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] Flat hook entries without type reach .codex/hooks.json without the required type: command

2 participants