From 44d445f9ae4eb501e67d2dcc040985eb981c4c4b Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Mon, 26 Oct 2026 00:17:04 +0100 Subject: [PATCH] fix(setup): load the upgrade hook in Codex Codex never ran magpie-setup's session-start upgrade check. With no Codex manifest, Codex read the plugin's hooks/hooks.json, whose `$schema` and `modules` keys (Claude Code mods) its strict hooks-file schema rejects, so no hook loaded and every start logged a parse warning; magpie-utilities hit the same warning. The root .codex-plugin hook #1829 re-pointed lacked the inline `hooks` wrapper, so Codex ignored it, and no catalogue installs the root plugin. - magpie-setup and magpie-utilities get a complete .codex-plugin/plugin.json pointing at a clean hooks/codex-hooks.json; magpie-setup's runs check-upgrade.sh as `/bin/bash -p` on startup, magpie-utilities' is empty. - The root manifest's hook is removed. - check-family-plugins generates and pins both files and fails any Codex-catalogued plugin whose Codex-visible hooks file has keys other than `description` and `hooks`. - The session-hook tests cover the Codex wiring; the marketplace docs describe it and Codex's /hooks trust step; uninstall names Codex's real `plugin remove` verb. Checked live with codex 0.154.0's app-server `hooks/list`: the hook loads from codex-hooks.json with no warnings, untrusted until reviewed. Generated-by: Claude Opus 5 --- .codex-plugin/plugin.json | 15 +- docs/setup/marketplace.md | 26 +- .../magpie-setup/.codex-plugin/plugin.json | 14 ++ plugins/magpie-setup/hooks/check-upgrade.sh | 13 +- plugins/magpie-setup/hooks/codex-hooks.json | 17 ++ .../magpie-setup/skills/setup/uninstall.md | 2 +- .../.codex-plugin/plugin.json | 14 ++ .../magpie-utilities/hooks/codex-hooks.json | 4 + projects/magpie/release-management-config.md | 9 +- tools/dev/check-family-plugins.py | 234 ++++++++++++++++-- tools/dev/tests/test_check_family_plugins.py | 101 ++++++++ .../tests/test_session_hook.py | 47 +++- 12 files changed, 426 insertions(+), 70 deletions(-) create mode 100644 plugins/magpie-setup/.codex-plugin/plugin.json create mode 100644 plugins/magpie-setup/hooks/codex-hooks.json create mode 100644 plugins/magpie-utilities/.codex-plugin/plugin.json create mode 100644 plugins/magpie-utilities/hooks/codex-hooks.json diff --git a/.codex-plugin/plugin.json b/.codex-plugin/plugin.json index e0791993f..3fee35857 100644 --- a/.codex-plugin/plugin.json +++ b/.codex-plugin/plugin.json @@ -2,18 +2,5 @@ "name": "magpie", "version": "0.9.0.dev202610251812", "description": "Apache Magpie — agentic skills for maintaining open-source projects.", - "skills": "./skills", - "hooks": { - "SessionStart": [ - { - "hooks": [ - { - "type": "command", - "command": "/bin/bash -p \"${PLUGIN_ROOT}/plugins/magpie-setup/hooks/check-upgrade.sh\"", - "timeout": 10 - } - ] - } - ] - } + "skills": "./skills" } diff --git a/docs/setup/marketplace.md b/docs/setup/marketplace.md index b8faa5bfb..aba9dfd25 100644 --- a/docs/setup/marketplace.md +++ b/docs/setup/marketplace.md @@ -335,16 +335,27 @@ When step 1 wins, `.codex-plugin/plugin.json` is not ignored — it is merged over the root manifest as an **overlay**, so it carries Codex-specific fields rather than the whole manifest. -For this repo that resolves two different ways: +For this repo that resolves three different ways: | Installed root | Manifest Codex actually reads | |---|---| -| the repo root | root [`plugin.json`](../../plugin.json) — its AP1 `$schema` matches — with `.codex-plugin/plugin.json` merged over it | -| `plugins/magpie-/` | `.claude-plugin/plugin.json`, reached by fallback: those roots carry neither a `plugin.json` nor a `.codex-plugin/` | +| the repo root | root [`plugin.json`](../../plugin.json) — its AP1 `$schema` matches — with `.codex-plugin/plugin.json` merged over it. No catalogue installs this root as a Codex plugin | +| `plugins/magpie-setup/`, `plugins/magpie-utilities/` | their own `.codex-plugin/plugin.json`, read **instead of** `.claude-plugin/plugin.json` (an overlay applies only over an AP1 root manifest), so it is a complete manifest: the same name, description and shared metadata, `skills`, and a `hooks` path — see below | +| every other `plugins/magpie-/` | `.claude-plugin/plugin.json`, reached by fallback: those roots carry neither a `plugin.json` nor a `.codex-plugin/` | -The second row is why the ten family plugins install into Codex at all despite +The last row is why the family plugins install into Codex at all despite carrying only a Claude-Code manifest. +**Hooks.** +Codex takes a plugin's hooks from its manifest's `hooks` entry, or, with none, from `hooks/hooks.json`. +It parses each hook file strictly, as an object with only `description` and `hooks`. +Any other top-level key rejects the whole file: Codex reports a load warning and runs none of its hooks. +The `hooks/hooks.json` of `magpie-setup` and `magpie-utilities` belongs to Claude Code and carries `$schema` and `modules` for the [Claude Code mods](../when-to-use-mods.md), so Codex cannot use it. +Each of those two plugins therefore has a `.codex-plugin/plugin.json` whose `hooks` names `./hooks/codex-hooks.json`, a file in the shape Codex parses. +Claude Code reads neither file and keeps using `hooks/hooks.json`. +For `magpie-setup` that file wires the [upgrade check](#automatic-upgrade-detection); for `magpie-utilities` it is empty, which only stops Codex from warning about `hooks/hooks.json` on every start. +`tools/dev/check-family-plugins.py` generates both files with `--fix`, and fails any plugin in the Codex catalogue whose Codex-visible hook file carries another top-level key. + > [!WARNING] > A plugin root's `plugin.json` must be a regular file. If it is a **symlink**, > Codex's resolver returns no manifest at all — it does *not* fall back to @@ -610,7 +621,7 @@ automatic while the *changes* stay confirmed. | Agent | Mechanism | |---|---| | **Claude Code** | `SessionStart` hook [`plugins/magpie-setup/hooks/check-upgrade.sh`](../../plugins/magpie-setup/hooks/check-upgrade.sh) compares the installed version to a marker in the plugin's persistent data dir and prompts on change. Deterministic. | -| **Codex CLI** | The same [`plugins/magpie-setup/hooks/check-upgrade.sh`](../../plugins/magpie-setup/hooks/check-upgrade.sh), wired inline in the `hooks` block of the root plugin's [`.codex-plugin/plugin.json`](../../.codex-plugin/plugin.json) as `/bin/bash -p "${PLUGIN_ROOT}/plugins/magpie-setup/hooks/check-upgrade.sh"` — Codex uses the same event schema and the same `SessionStart` event. For the root plugin `PLUGIN_ROOT` is the repo root, where the script finds the version manifests and the `skills/setup` symlink it needs. Codex sets `PLUGIN_ROOT`/`PLUGIN_DATA` (and the `CLAUDE_*` pair for compatibility), which the script reads. See the caveat below. | +| **Codex CLI** | The same [`plugins/magpie-setup/hooks/check-upgrade.sh`](../../plugins/magpie-setup/hooks/check-upgrade.sh), on the same `startup` `SessionStart` event, wired in [`plugins/magpie-setup/hooks/codex-hooks.json`](../../plugins/magpie-setup/hooks/codex-hooks.json) as `/bin/bash -p "${PLUGIN_ROOT}/hooks/check-upgrade.sh"`. The plugin's [`.codex-plugin/plugin.json`](../../plugins/magpie-setup/.codex-plugin/plugin.json) names that file, because Codex cannot parse the Claude Code `hooks/hooks.json` (see [Which manifest Codex reads](#which-manifest-codex-reads)). `PLUGIN_ROOT` is the installed `magpie-setup` root, where the script finds its manifests and `skills/setup`. Codex sets `PLUGIN_ROOT`/`PLUGIN_DATA` (and the `CLAUDE_*` pair for compatibility), which the script reads. It runs only once you have trusted it — see the caveat below. | | **VS Code / Copilot, Cursor, Kiro (AP1)** | None. Agent Plugins 1.0 specifies no hook component and VS Code ignores client extension directories, so there is nothing to fire. Run `/magpie-setup:complete-upgrade` after updating. | | **Gemini CLI** | No lifecycle hook; the extension context file [`GEMINI.md`](../../GEMINI.md) instructs the agent to compare the extension version to a recorded marker and prompt on change (LLM-driven, advisory). | | Other agents | Run `/magpie-setup:complete-upgrade` manually after updating the package. | @@ -618,7 +629,8 @@ automatic while the *changes* stay confirmed. > [!IMPORTANT] > **Codex hooks require a separate trust decision.** Current Codex supports plugin-local hooks, but skips non-managed definitions until the operator reviews them in `/hooks`. > Project trust or plugin installation alone does not activate them. -> Review the upgrade hook and the guard and vetted-ops hooks after installation or a definition change; then verify they actually run. +> After installing `magpie-setup`, open `/hooks`, review its `SessionStart` upgrade hook (source `hooks/codex-hooks.json`), and trust it; it then runs from the next new session. +> Do the same for the guard and vetted-ops hooks, and again after any update that changes a hook definition, since Codex keys trust to the definition's hash; then verify they actually run. > Older versions affected by [openai/codex#16430](https://github.com/openai/codex/issues/16430) still need manual upgrade checks. > See [the native hook contract](../adapters/codex.md#deterministic-guard-rules) and [OpenAI’s hook documentation](https://learn.chatgpt.com/docs/hooks). @@ -765,7 +777,7 @@ documentation**; what varies is whether it has also been exercised against a |---|---|---| | root `plugin.json` | [Agent Plugins 1.0.0 spec](https://github.com/agentplugins/agent-plugins-spec/blob/main/spec/1.0.0.md) + [`plugin.schema.json`](https://agent-plugins.org/schemas/1.0.0/plugin.schema.json) | Conforms to the published closed schema; enforced by `check-family-plugins.py`. Also the manifest Codex resolves first for the repo root — see [Which manifest Codex reads](#which-manifest-codex-reads) | | `.claude-plugin/*` | Claude Code plugins reference | Verified live — `claude plugin validate . --strict` passes with 0 warnings; a family plugin installs and loads from a local marketplace replica | -| `.codex-plugin/plugin.json`, `.agents/plugins/marketplace.json` | The `codex` binary's own source and enums — native hook behavior is documented in [OpenAI’s hook reference](https://learn.chatgpt.com/docs/hooks); manifest-resolution order was checked against [`plugin_namespace.rs`][codex-resolve] and [`protocol.rs`][codex-paths] | **Verified live** — `codex plugin marketplace add` + `plugin list` against codex 0.154.0. The first live run is what caught the invented `policy` values the documentation check could not; the enums are now enforced by `check-family-plugins.py`. Note `.codex-plugin/plugin.json` is an **overlay** on the root manifest, not the manifest itself. See the hook-trust note above | +| `.codex-plugin/plugin.json`, `.agents/plugins/marketplace.json` | The `codex` binary's own source and enums — native hook behavior is documented in [OpenAI’s hook reference](https://learn.chatgpt.com/docs/hooks); manifest-resolution order was checked against [`plugin_namespace.rs`][codex-resolve] and [`protocol.rs`][codex-paths] | **Verified live** — `codex plugin marketplace add` + `plugin list` against codex 0.154.0. The first live run is what caught the invented `policy` values the documentation check could not; the enums are now enforced by `check-family-plugins.py`. Note the root `.codex-plugin/plugin.json` is an **overlay** on the root manifest, not the manifest itself, while a family plugin's `.codex-plugin/plugin.json` is the whole manifest. The `magpie-setup` upgrade hook was checked live with the app server's `hooks/list` against codex 0.154.0: it loads from `hooks/codex-hooks.json` with no warning, untrusted until reviewed, where the Claude Code `hooks/hooks.json` failed with ``unknown field `$schema` ``. See the hook-trust note above | | root `marketplace.json` | Copilot / VS Code plugin marketplace docs | Legacy-format catalog, explicitly still supported alongside AP1. Not yet live-installed | | `gemini-extension.json` | Gemini CLI extensions docs | Follows the published schema. Google has joined the AP1 TSC but has published no migration for this file — keep both | | `apm.yml` | `microsoft/apm` schema **v0.1** | Pre-1.0 and the most likely to churn; re-check before publish | diff --git a/plugins/magpie-setup/.codex-plugin/plugin.json b/plugins/magpie-setup/.codex-plugin/plugin.json new file mode 100644 index 000000000..ab674288a --- /dev/null +++ b/plugins/magpie-setup/.codex-plugin/plugin.json @@ -0,0 +1,14 @@ +{ + "name": "magpie-setup", + "description": "Apache Magpie \u2014 framework install/maintenance: install (adopt), upgrade, verify, override, status, secure-agent setup, shared-config sync.", + "version": "0.9.0.dev202610251812", + "author": { + "name": "Apache Magpie", + "url": "https://magpie.apache.org/" + }, + "homepage": "https://magpie.apache.org/", + "repository": "https://github.com/apache/magpie", + "license": "Apache-2.0", + "skills": "./skills", + "hooks": "./hooks/codex-hooks.json" +} diff --git a/plugins/magpie-setup/hooks/check-upgrade.sh b/plugins/magpie-setup/hooks/check-upgrade.sh index cb377671f..c2b1ff905 100755 --- a/plugins/magpie-setup/hooks/check-upgrade.sh +++ b/plugins/magpie-setup/hooks/check-upgrade.sh @@ -2,11 +2,14 @@ # SPDX-License-Identifier: Apache-2.0 # https://www.apache.org/licenses/LICENSE-2.0 # -# Session-start hook for the Apache Magpie plugin. Wired by the Claude Code -# magpie-setup plugin (hooks/hooks.json, SessionStart) and by the root Codex CLI -# plugin (.codex-plugin/plugin.json, same event schema), whose PLUGIN_ROOT is -# the repo root. Agent Plugins 1.0 defines no hook component, so an AP1-only client -# does not run this at all — see docs/setup/marketplace.md. +# Session-start hook for the Apache Magpie plugin. Wired by the magpie-setup +# plugin twice, with the same event schema: for Claude Code in hooks/hooks.json, +# and for Codex CLI in hooks/codex-hooks.json, which .codex-plugin/plugin.json +# names because Codex cannot parse the Claude Code mods' keys in hooks.json. +# Either way the plugin root is this plugin's own root. Codex runs it only once +# the user has trusted it in `/hooks`. Agent Plugins 1.0 defines no hook +# component, so an AP1-only client does not run this at all — see +# docs/setup/marketplace.md. # # Also warns, once per session, when the isolated setup is not configured on # this machine (see `setup_preflight.isolated.session_start`). diff --git a/plugins/magpie-setup/hooks/codex-hooks.json b/plugins/magpie-setup/hooks/codex-hooks.json new file mode 100644 index 000000000..d09b823a8 --- /dev/null +++ b/plugins/magpie-setup/hooks/codex-hooks.json @@ -0,0 +1,17 @@ +{ + "description": "Apache Magpie \u2014 the upgrade check, for Codex. Claude Code reads hooks.json.", + "hooks": { + "SessionStart": [ + { + "matcher": "startup", + "hooks": [ + { + "type": "command", + "command": "/bin/bash -p \"${PLUGIN_ROOT}/hooks/check-upgrade.sh\"", + "timeout": 10 + } + ] + } + ] + } +} diff --git a/plugins/magpie-setup/skills/setup/uninstall.md b/plugins/magpie-setup/skills/setup/uninstall.md index e44cdd822..e67a65985 100644 --- a/plugins/magpie-setup/skills/setup/uninstall.md +++ b/plugins/magpie-setup/skills/setup/uninstall.md @@ -40,7 +40,7 @@ explicit confirmation before any write. > install — the default path — put nothing in the repo, so there > is nothing here to reverse: it is removed with the client's own > verb (`/plugin uninstall @apache-magpie` in Claude -> Code, `codex plugin uninstall magpie`, +> Code, `codex plugin remove @apache-magpie`, > `gemini extensions uninstall magpie`). If the repo has no > `.apache-magpie.lock` and Magpie is plugin-installed, print > that command instead of a snapshot removal plan. diff --git a/plugins/magpie-utilities/.codex-plugin/plugin.json b/plugins/magpie-utilities/.codex-plugin/plugin.json new file mode 100644 index 000000000..b9cf89b34 --- /dev/null +++ b/plugins/magpie-utilities/.codex-plugin/plugin.json @@ -0,0 +1,14 @@ +{ + "name": "magpie-utilities", + "description": "Apache Magpie \u2014 framework meta-skills: write-skill, optimize-skill, skill-reconciler, list-skills.", + "version": "0.9.0.dev202610251812", + "author": { + "name": "Apache Magpie", + "url": "https://magpie.apache.org/" + }, + "homepage": "https://magpie.apache.org/", + "repository": "https://github.com/apache/magpie", + "license": "Apache-2.0", + "skills": "./skills", + "hooks": "./hooks/codex-hooks.json" +} diff --git a/plugins/magpie-utilities/hooks/codex-hooks.json b/plugins/magpie-utilities/hooks/codex-hooks.json new file mode 100644 index 000000000..b0fac4109 --- /dev/null +++ b/plugins/magpie-utilities/hooks/codex-hooks.json @@ -0,0 +1,4 @@ +{ + "description": "Apache Magpie \u2014 no Codex hooks. hooks.json holds Claude Code mods only.", + "hooks": {} +} diff --git a/projects/magpie/release-management-config.md b/projects/magpie/release-management-config.md index e284edec1..fe74fa6a7 100644 --- a/projects/magpie/release-management-config.md +++ b/projects/magpie/release-management-config.md @@ -76,7 +76,7 @@ mandatory ASF approval + announce mechanisms (`dev-list-vote`, | `git_upstream_remote` | `upstream` | | `release_planning_issue_template` | *(none — uses the `release-prepare` default template)* | | `release_branch_base` | `main` | -| `version_manifest_files` | `pyproject.toml`, `uv.lock`, `.claude-plugin/plugin.json`, `.claude-plugin/marketplace.json`, `plugin.json` (Agent Plugins 1.0), `.codex-plugin/plugin.json`, `gemini-extension.json`, `apm.yml`, and the ten generated `plugins/magpie-/.claude-plugin/plugin.json` manifests | +| `version_manifest_files` | `pyproject.toml`, `uv.lock`, `.claude-plugin/plugin.json`, `.claude-plugin/marketplace.json`, `plugin.json` (Agent Plugins 1.0), `.codex-plugin/plugin.json`, `gemini-extension.json`, `apm.yml`, the ten generated `plugins/magpie-/.claude-plugin/plugin.json` manifests, and the generated Codex manifests `plugins/magpie-setup/.codex-plugin/plugin.json` and `plugins/magpie-utilities/.codex-plugin/plugin.json` | `pyproject.toml`'s `project.version` is the **single authority** for the framework version; every other file above mirrors it verbatim, including a @@ -116,9 +116,10 @@ The version flows outward in two hops, neither of them hand-edited: (`.claude-plugin/plugin.json`, the Agent Plugins 1.0 `plugin.json`, `.codex-plugin/plugin.json`, `gemini-extension.json`, `apm.yml`); 2. `.claude-plugin/plugin.json` → the ten generated - `plugins/magpie-/.claude-plugin/plugin.json` manifests and the - family entries in `.claude-plugin/marketplace.json`, which also inherit - `author`, `homepage`, `repository`, and `license`. + `plugins/magpie-/.claude-plugin/plugin.json` manifests, the two + generated `plugins/magpie-{setup,utilities}/.codex-plugin/plugin.json` + manifests, and the family entries in `.claude-plugin/marketplace.json`, + which also inherit `author`, `homepage`, `repository`, and `license`. So a bump is one edit plus one command: diff --git a/tools/dev/check-family-plugins.py b/tools/dev/check-family-plugins.py index a492fcd12..f55e1b03d 100644 --- a/tools/dev/check-family-plugins.py +++ b/tools/dev/check-family-plugins.py @@ -12,8 +12,15 @@ `pyproject.toml`'s `project.version` verbatim — including a `.devN` suffix; - `magpie-setup`'s manifest wires the `hooks/check-upgrade.sh` SessionStart hook (which must exist) -- it rides on the one plugin the recommended floor - always installs -- and the root `.codex-plugin/plugin.json` wires the same - script for Codex, through `plugins/magpie-setup/` from the repo root; + always installs. For Codex, `plugins/magpie-setup/.codex-plugin/plugin.json` + (a complete manifest with the same shared metadata) names + `hooks/codex-hooks.json`, which wires the same script in the + `{description, hooks}` shape Codex parses; the root `.codex-plugin/plugin.json` + wires no hook, since nothing installs the repo root as a Codex plugin; +- every hook definition a plugin in the Codex catalogue exposes (its manifest's + `hooks` entry, else `hooks/hooks.json`) has no top-level key but + `description` and `hooks` -- Codex rejects the whole file otherwise, so a + Claude Code mod's `$schema` / `modules` there silently drops its hooks; - the root `plugin.json` conforms to Agent Plugins 1.0 — the pinned `$schema`, the name pattern, the closed ten-field set (so a Claude-only component path never leaks in), and the same shared metadata as the Claude manifest; @@ -79,23 +86,49 @@ } ] } -# The same script, wired for Codex in the root plugin's `.codex-plugin/plugin.json` -# (the overlay Codex merges over the root `plugin.json`). `${PLUGIN_ROOT}` is the -# repo root there, so the path runs through `plugins/magpie-setup/`; the script -# finds the root's manifests and its `skills/setup` symlink from that root. -CODEX_MANIFEST = Path(".codex-plugin/plugin.json") -CODEX_HOOKS = { - "SessionStart": [ - { - "hooks": [ +# The root plugin's Codex manifest. Nothing installs the repo root as a Codex +# plugin (the Codex catalogue lists the family and substrate plugins only), so +# it declares no `hooks`: the upgrade check is wired on magpie-setup, below. +ROOT_CODEX_MANIFEST = Path(".codex-plugin/plugin.json") +# Codex loads a plugin's hooks from its manifest's `hooks` entry when there is +# one, else from `hooks/hooks.json`, and parses either as a strict +# `{description, hooks}` object: any other top-level key rejects the file, so +# Codex reports a load warning and runs none of its hooks. A family plugin's +# `hooks/hooks.json` belongs to Claude Code and carries `$schema` and `modules` +# for its mods, so such a plugin gets its own `.codex-plugin/plugin.json` -- +# which Codex reads in preference to `.claude-plugin/plugin.json`, as the whole +# manifest rather than an overlay -- pointing at a Codex-clean hook file. +# Claude Code reads neither file and keeps using `hooks/hooks.json` alone. +CODEX_HOOK_KEYS = frozenset({"description", "hooks"}) +CODEX_HOOKS_FILE = Path("hooks/codex-hooks.json") # relative to the plugin root +# The exact content of each such plugin's Codex hook file. magpie-setup carries +# the upgrade check, wired as for Claude Code (`startup` only, the fixed +# `/bin/bash -p`); Codex's `PLUGIN_ROOT` is the plugin's own root, where the +# script finds its manifests and `skills/setup`. magpie-utilities has no Codex +# hook: its file exists so Codex does not fall back to the Claude-only +# `hooks/hooks.json` and warn about it on every start. +CODEX_FAMILY_HOOKS = { + "magpie-setup": { + "description": "Apache Magpie — the upgrade check, for Codex. Claude Code reads hooks.json.", + "hooks": { + "SessionStart": [ { - "type": "command", - "command": f'/bin/bash -p "${{PLUGIN_ROOT}}/{UPGRADE_HOOK.as_posix()}"', - "timeout": 10, + "matcher": "startup", + "hooks": [ + { + "type": "command", + "command": '/bin/bash -p "${PLUGIN_ROOT}/hooks/check-upgrade.sh"', + "timeout": 10, + } + ], } - ], - } - ] + ] + }, + }, + "magpie-utilities": { + "description": "Apache Magpie — no Codex hooks. hooks.json holds Claude Code mods only.", + "hooks": {}, + }, } # Every skill's pre-flight runs `setup_preflight`, and `setup config` installs it # from the plugin on a marketplace install, so the package has to be inside @@ -211,7 +244,7 @@ ECOSYSTEM_MANIFESTS = ( ROOT_MANIFEST, AP1_MANIFEST, - Path(".codex-plugin/plugin.json"), + ROOT_CODEX_MANIFEST, Path("gemini-extension.json"), Path("apm.yml"), ) @@ -964,6 +997,148 @@ def check_codex_policy(path: Path, entries: list) -> list[str]: return errors +def codex_family_manifest(name: str, description: str, shared: dict) -> dict: + """The `.codex-plugin/plugin.json` `--fix` writes for a plugin in `CODEX_FAMILY_HOOKS`. + + A complete manifest, not an overlay: Codex reads it *instead of* + `.claude-plugin/plugin.json`, so it repeats that manifest's name, + description, shared metadata and skills path, and adds only `hooks`. + """ + return { + "name": name, + "description": description, + **shared, + "skills": "./skills", + "hooks": f"./{CODEX_HOOKS_FILE.as_posix()}", + } + + +def check_codex_family_hooks(shared: dict) -> list[str]: + """magpie-setup's upgrade check reaches Codex; the root Codex manifest wires no hook.""" + errors: list[str] = [] + root, err = load_json(ROOT_CODEX_MANIFEST) + if err: + errors.append(err) + elif isinstance(root, dict) and "hooks" in root: + errors.append( + f"{ROOT_CODEX_MANIFEST}: declares 'hooks', but no catalogue installs the repo root " + f"as a Codex plugin; the upgrade check is wired in " + f"{PLUGINS / 'magpie-setup' / CODEX_HOOKS_FILE} (remove it, or run --fix)" + ) + for name, want_hooks in sorted(CODEX_FAMILY_HOOKS.items()): + pdir = PLUGINS / name + claude, claude_err = load_json(pdir / ".claude-plugin" / "plugin.json") + if claude_err: + errors.append(claude_err) + continue + path = pdir / ".codex-plugin" / "plugin.json" + if not path.is_file(): + errors.append( + f"{path}: missing -- without it Codex reads hooks/hooks.json, which it cannot " + "parse (regenerate with --fix)" + ) + else: + data, data_err = load_json(path) + want = codex_family_manifest(name, claude.get("description", ""), shared) + if data_err: + errors.append(data_err) + elif shared and data != want: + errors.append( + f"{path}: does not match the Codex manifest the framework expects -- " + f"{pdir / '.claude-plugin' / 'plugin.json'}'s name, description and shared " + f"metadata, './skills', and 'hooks': './{CODEX_HOOKS_FILE.as_posix()}' " + "(regenerate with --fix)" + ) + hooks_path = pdir / CODEX_HOOKS_FILE + hooks, hooks_err = load_json(hooks_path) + if hooks_err: + errors.append(hooks_err) + elif hooks != want_hooks: + errors.append( + f"{hooks_path}: does not match the Codex hook wiring the framework expects " + "(regenerate with --fix)" + ) + return errors + + +def write_codex_family_hooks(name: str, description: str, shared: dict) -> None: + """(Re)generate a plugin's `.codex-plugin/plugin.json` and its Codex hook file.""" + pdir = PLUGINS / name + (pdir / ".codex-plugin").mkdir(parents=True, exist_ok=True) + (pdir / ".codex-plugin" / "plugin.json").write_text( + json.dumps(codex_family_manifest(name, description, shared), indent=2) + "\n", encoding="utf-8" + ) + (pdir / CODEX_HOOKS_FILE).parent.mkdir(parents=True, exist_ok=True) + (pdir / CODEX_HOOKS_FILE).write_text( + json.dumps(CODEX_FAMILY_HOOKS[name], indent=2) + "\n", encoding="utf-8" + ) + + +def codex_hook_sources(pdir: Path) -> list[tuple[str, object]]: + """The hook definitions Codex loads for the plugin at `pdir`, as (label, value). + + Mirrors Codex's own resolution: the first of `.codex-plugin/plugin.json` and + `.claude-plugin/plugin.json` (no plugin here carries an Agent Plugins root + manifest), then its `hooks` entry -- a path, a list of paths, an inline + object, or a list of them -- or, with none, `hooks/hooks.json` if present. + A value that is not JSON comes back as an error string. + """ + manifest = None + for rel in (".codex-plugin/plugin.json", ".claude-plugin/plugin.json"): + if (pdir / rel).is_file(): + manifest = pdir / rel + break + if manifest is None: + return [] + data, err = load_json(manifest) + if err: + return [(str(manifest), err)] + hooks = data.get("hooks") if isinstance(data, dict) else None + if hooks is None: + default = pdir / "hooks" / "hooks.json" + hooks = [] if not default.is_file() else ["./hooks/hooks.json"] + entries = hooks if isinstance(hooks, list) else [hooks] + sources: list[tuple[str, object]] = [] + for index, entry in enumerate(entries): + if isinstance(entry, str): + path = pdir / entry.removeprefix("./") + value, value_err = load_json(path) + sources.append((str(path), value_err if value_err else value)) + else: + sources.append((f"{manifest}#hooks[{index}]", entry)) + return sources + + +def check_codex_hook_files() -> list[str]: + """Every hook definition a Codex-catalogued plugin exposes parses as Codex's `HooksFile`. + + Codex deserialises each with unknown fields denied, so a Claude-Code-only key + (`$schema`, `modules`) is not ignored: it rejects the file and every hook in it. + """ + catalog, err = load_json(CODEX_CATALOG) + if err or not isinstance(catalog, dict): + return [] # reported by check_client_catalogs + errors: list[str] = [] + for entry in catalog.get("plugins", []): + source = entry.get("source") if isinstance(entry, dict) else None + rel = source.get("path") if isinstance(source, dict) else None + if not isinstance(rel, str): + continue + for label, value in codex_hook_sources(Path(rel.removeprefix("./"))): + if isinstance(value, str): + errors.append(value) + elif not isinstance(value, dict): + errors.append(f"{label}: Codex expects a hooks object, found {type(value).__name__}") + elif extra := sorted(set(value) - CODEX_HOOK_KEYS): + errors.append( + f"{label}: top-level key(s) {', '.join(extra)} -- Codex accepts only " + f"{', '.join(sorted(CODEX_HOOK_KEYS))} and rejects the whole file, so none " + f"of its hooks run. Give the plugin a .codex-plugin/plugin.json whose " + f"'hooks' names a clean file (see CODEX_FAMILY_HOOKS)" + ) + return errors + + def root_metadata() -> tuple[dict, list[str]]: """The subset of the root manifest that the family plugins inherit.""" data, err = load_json(ROOT_MANIFEST) @@ -1119,14 +1294,8 @@ def check(fam: dict[str, set[str]]) -> list[str]: else: if setup_data is not None and "check-upgrade.sh" not in json.dumps(setup_data.get("hooks", {})): errors.append(f"{setup_manifest}: SessionStart hook does not reference check-upgrade.sh") - codex_data, codex_err = load_json(CODEX_MANIFEST) - if codex_err: - errors.append(codex_err) - elif isinstance(codex_data, dict) and codex_data.get("hooks") != CODEX_HOOKS: - errors.append( - f"{CODEX_MANIFEST}: 'hooks' must be exactly {json.dumps(CODEX_HOOKS)} " - "(the root plugin's PLUGIN_ROOT is the repo root, and the hook runs as `/bin/bash -p `)" - ) + errors += check_codex_family_hooks(shared) + errors += check_codex_hook_files() errors += check_setup_preflight_package() errors += welcome_families("check-constant") @@ -1367,6 +1536,19 @@ def fix(fam: dict[str, set[str]]) -> int: (pdir / ".claude-plugin" / "plugin.json").write_text( json.dumps(manifest, indent=2) + "\n", encoding="utf-8" ) + if name in CODEX_FAMILY_HOOKS: + write_codex_family_hooks(name, manifest["description"], shared) + + # Nothing installs the repo root as a Codex plugin, so its manifest wires no hook. + root_codex, root_codex_err = load_json(ROOT_CODEX_MANIFEST) + if root_codex_err: + print(f" - {root_codex_err}", file=sys.stderr) + return 1 + if isinstance(root_codex, dict) and "hooks" in root_codex: + del root_codex["hooks"] + ROOT_CODEX_MANIFEST.write_text( + json.dumps(root_codex, indent=2, ensure_ascii=False) + "\n", encoding="utf-8" + ) market, market_err = load_json(MARKETPLACE) if market_err: diff --git a/tools/dev/tests/test_check_family_plugins.py b/tools/dev/tests/test_check_family_plugins.py index 4c7e50dac..b0270665d 100644 --- a/tools/dev/tests/test_check_family_plugins.py +++ b/tools/dev/tests/test_check_family_plugins.py @@ -579,3 +579,104 @@ def test_the_security_tools_plugin_ships_the_forwarder_registry(): copies = mod.SUBSTRATE_PLUGINS["magpie-security-tools"]["copies"] assert "tools/forwarder-relay/README.md" in copies assert "tools/gmail/asf-relay.md" in copies + + +# --------------------------------------------------------------------------- +# Codex loads magpie-setup's upgrade hook from a hook file it can parse +# --------------------------------------------------------------------------- + + +@pytest.fixture +def codex_tree(tmp_path, monkeypatch): + """The plugins with Codex hook files, generated the way ``--fix`` does, and + a Codex catalogue listing them.""" + monkeypatch.chdir(tmp_path) + (tmp_path / ".codex-plugin").mkdir() + (tmp_path / ".codex-plugin" / "plugin.json").write_text( + json.dumps({"name": "magpie", "version": SHARED["version"], "skills": "./skills"}), encoding="utf-8" + ) + for name in mod.CODEX_FAMILY_HOOKS: + pdir = tmp_path / "plugins" / name + (pdir / ".claude-plugin").mkdir(parents=True) + (pdir / "hooks").mkdir() + (pdir / ".claude-plugin" / "plugin.json").write_text( + json.dumps({"name": name, "description": f"{name} skills", **SHARED, "skills": "./skills"}), + encoding="utf-8", + ) + # Claude Code's hook file, which Codex cannot parse. + (pdir / "hooks" / "hooks.json").write_text( + json.dumps({"$schema": "https://code.claude.com/schemas/hooks.json", "modules": ["./m.ts"]}), + encoding="utf-8", + ) + mod.write_codex_family_hooks(name, f"{name} skills", SHARED) + catalog = tmp_path / mod.CODEX_CATALOG + catalog.parent.mkdir(parents=True) + catalog.write_text( + json.dumps( + { + "plugins": [ + {"name": name, "source": {"source": "local", "path": f"./plugins/{name}"}} + for name in mod.CODEX_FAMILY_HOOKS + ] + } + ), + encoding="utf-8", + ) + return tmp_path + + +def _codex_manifest(tree: Path, name: str = "magpie-setup") -> Path: + return tree / "plugins" / name / ".codex-plugin" / "plugin.json" + + +def test_the_generated_codex_wiring_passes(codex_tree): + assert mod.check_codex_family_hooks(SHARED) == [] + assert mod.check_codex_hook_files() == [] + + +def test_the_codex_manifest_names_the_clean_hook_file(codex_tree): + manifest = json.loads(_codex_manifest(codex_tree).read_text(encoding="utf-8")) + assert manifest["hooks"] == "./hooks/codex-hooks.json" + assert manifest["skills"] == "./skills" + assert {k: manifest[k] for k in SHARED} == SHARED + hooks = json.loads((codex_tree / "plugins/magpie-setup/hooks/codex-hooks.json").read_text()) + command = hooks["hooks"]["SessionStart"][0]["hooks"][0]["command"] + assert command == '/bin/bash -p "${PLUGIN_ROOT}/hooks/check-upgrade.sh"' + + +def test_a_root_codex_hook_is_reported(codex_tree): + _rewrite(codex_tree / ".codex-plugin" / "plugin.json", hooks={"SessionStart": []}) + assert any("no catalogue installs the repo root" in e for e in mod.check_codex_family_hooks(SHARED)) + + +def test_a_missing_codex_manifest_is_reported(codex_tree): + _codex_manifest(codex_tree).unlink() + assert any("missing" in e for e in mod.check_codex_family_hooks(SHARED)) + # ...and Codex then falls back to the Claude-only hooks.json. + assert any("$schema" in e for e in mod.check_codex_hook_files()) + + +def test_a_codex_manifest_left_at_an_old_version_is_reported(codex_tree): + _rewrite(_codex_manifest(codex_tree), version="0.0.1") + assert any("does not match the Codex manifest" in e for e in mod.check_codex_family_hooks(SHARED)) + + +def test_an_altered_codex_hook_file_is_reported(codex_tree): + _rewrite(codex_tree / "plugins/magpie-setup/hooks/codex-hooks.json", hooks={}) + assert any("does not match the Codex hook wiring" in e for e in mod.check_codex_family_hooks(SHARED)) + + +def test_a_claude_only_key_in_a_codex_hook_file_is_reported(codex_tree): + _rewrite(codex_tree / "plugins/magpie-setup/hooks/codex-hooks.json", modules=["./m.ts"]) + assert any("modules" in e and "rejects the whole file" in e for e in mod.check_codex_hook_files()) + + +def test_an_inline_codex_hook_with_a_claude_only_key_is_reported(codex_tree): + _rewrite(_codex_manifest(codex_tree), hooks={"$schema": "x", "hooks": {}}) + assert any("#hooks[0]" in e for e in mod.check_codex_hook_files()) + + +def test_every_real_codex_plugin_exposes_parseable_hooks(monkeypatch): + monkeypatch.chdir(REPO_ROOT) + assert mod.check_codex_hook_files() == [] + assert mod.check_codex_family_hooks(mod.root_metadata()[0]) == [] diff --git a/tools/setup-preflight/tests/test_session_hook.py b/tools/setup-preflight/tests/test_session_hook.py index 5ddbd7aea..fa894af68 100644 --- a/tools/setup-preflight/tests/test_session_hook.py +++ b/tools/setup-preflight/tests/test_session_hook.py @@ -138,38 +138,59 @@ def test_pythonpath_does_not_reach_the_preflight(project: Path, tmp_path: Path) HOOKS_JSON = PLUGIN_ROOT / "hooks" / "hooks.json" +# Codex cannot parse hooks.json (its `$schema` and `modules` keys are Claude Code's), +# so the plugin's Codex manifest names a hook file of its own. +CODEX_MANIFEST = PLUGIN_ROOT / ".codex-plugin" / "plugin.json" +CODEX_HOOKS_JSON = PLUGIN_ROOT / "hooks" / "codex-hooks.json" +# Each client's hook file, and the variables it hands the hook: Claude Code the +# `CLAUDE_*` pair, Codex `PLUGIN_ROOT` / `PLUGIN_DATA` (it sets the `CLAUDE_*` +# pair too, but the hook must not depend on that). +CLIENTS = { + "claude-code": (HOOKS_JSON, "CLAUDE_PLUGIN_ROOT", "CLAUDE_PLUGIN_DATA"), + "codex": (CODEX_HOOKS_JSON, "PLUGIN_ROOT", "PLUGIN_DATA"), +} + + +def _wired_command(hooks_file: Path = HOOKS_JSON) -> str: + data = json.loads(hooks_file.read_text(encoding="utf-8")) + return data["hooks"]["SessionStart"][0]["hooks"][0]["command"] -def _wired_command() -> str: - import json - - data = json.loads(HOOKS_JSON.read_text(encoding="utf-8")) - return data["hooks"]["SessionStart"][0]["hooks"][0]["command"] +def test_codex_loads_the_hook_from_its_own_file() -> None: + """Codex reads `.codex-plugin/plugin.json` first; its `hooks` must name a file Codex can parse.""" + manifest = json.loads(CODEX_MANIFEST.read_text(encoding="utf-8")) + assert manifest["hooks"] == "./hooks/codex-hooks.json" + assert set(json.loads(CODEX_HOOKS_JSON.read_text(encoding="utf-8"))) <= {"description", "hooks"} + assert "${PLUGIN_ROOT}/hooks/check-upgrade.sh" in _wired_command(CODEX_HOOKS_JSON) -def test_the_hook_runs_a_fixed_bash_in_privileged_mode() -> None: +@pytest.mark.parametrize("client", sorted(CLIENTS)) +def test_the_hook_runs_a_fixed_bash_in_privileged_mode(client: str) -> None: """`bash` and `#!/usr/bin/env bash` both resolve through PATH, which a project can set.""" - assert _wired_command().startswith("/bin/bash -p ") + assert _wired_command(CLIENTS[client][0]).startswith("/bin/bash -p ") assert HOOK.read_text(encoding="utf-8").splitlines()[0] == "#!/bin/bash -p" -def test_a_bash_on_path_is_never_run(project: Path, tmp_path: Path) -> None: - """The wired command, run by a shell as Claude Code runs it, ignores a bash on PATH.""" +@pytest.mark.parametrize("client", sorted(CLIENTS)) +def test_a_bash_on_path_is_never_run(project: Path, tmp_path: Path, client: str) -> None: + """The wired command, run by a shell as the client runs it, ignores a bash on PATH.""" + hooks_file, root_var, data_var = CLIENTS[client] bin_dir = tmp_path / "bin" bin_dir.mkdir() marker = tmp_path / "path-bash-ran" fake = bin_dir / "bash" fake.write_text(f"#!/bin/sh\ntouch {marker}\nexit 0\n") fake.chmod(0o755) + inherited = {k: v for k, v in os.environ.items() if k not in {"CLAUDE_PLUGIN_ROOT", "CLAUDE_PLUGIN_DATA"}} env = { - **os.environ, + **inherited, "PATH": f"{bin_dir}:{os.environ['PATH']}", - "CLAUDE_PLUGIN_ROOT": str(PLUGIN_ROOT), - "CLAUDE_PLUGIN_DATA": str(tmp_path / "data"), + root_var: str(PLUGIN_ROOT), + data_var: str(tmp_path / "data"), "CLAUDE_PROJECT_DIR": str(project), } done = subprocess.run( - ["/bin/sh", "-c", _wired_command()], + ["/bin/sh", "-c", _wired_command(hooks_file)], input="{}", capture_output=True, text=True,