From 89a3a6ac35cfe7c0cf68807aba74364c8627b362 Mon Sep 17 00:00:00 2001 From: antianqi Date: Tue, 25 Aug 2026 23:28:40 +0800 Subject: [PATCH 1/6] proposal: add detailed Hooks spec for io.minimax.mcode (companion to d86625d) Adds a companion proposal to proposals/hooks.md (commit d86625d) that records the twelve-event catalog, decision semantics, and field vocabulary actually shipped in @minimax-ai/code@0.2.4, plus the minimum registry-side scaffolding needed for MiniMax-Code-Plugins to enforce the proposal. This PR does not change the documented "not currently public" claim in docs/plugin-compatibility.md. Runtime conformance fixtures are still blocked on upstream acceptance of the portable Hooks proposal. Validation - scripts/lib/validation.mjs: new validateClientExtensions, validateHooksDocument, and validateHookEntry. Recognizes the io.minimax.mcode extension namespace statically; no Plugin code is ever executed. Reserved fields (type, shell, prompt, http, agent, script, function) are rejected. PLUGIN_ROOT and PLUGIN_DATA are reserved in env. - scripts/validate.mjs: unchanged; existing examples hello-mcode and hello-mcode-mcp continue to pass. The new example hello-mcode-hooks is recognized and validated. - smoke self-check: no hardcoded paths, literal tokens, or scaffold markers in any newly added file (record.mjs uses only PLUGIN_ROOT/PLUGIN_DATA and cross-platform node:path). Test evidence - test/validation.test.mjs: 5 new tests, all passing. * accepts a Hook entry with allowed field vocabulary and rejects reserved discriminators * accepts a Hooks document that targets the experimental io.minimax.mcode namespace * validatePluginDirectory picks up an io.minimax.mcode hooks extension without requiring it * validatePluginDirectory ignores a missing hooks extension * validatePluginDirectory rejects hooks.json with an unrecognized event - Full suite: 114/115 pass. The single failure is test/hosted-plugins.test.mjs:15, a pre-existing Windows-only assertion that hardcodes POSIX path separators; Linux CI is green. Design compliance - Agent Plugins 1.0 conformance preserved: Hooks remain an extension under io.minimax.mcode, not a root plugin.json field. The existing "rejects unsupported plugin capabilities in the manifest" test still passes. - Cross-platform: every path the example resolves comes from PLUGIN_ROOT or PLUGIN_DATA. No host-absolute literals, no drive letters, no /Users/ or /home/ paths. - Self-disclosure: SKILL.md, plugin.json description, and README each state no credentials, no network, no telemetry, no third-party services. - Companion (not replacement): this proposal explicitly defers to proposals/hooks.md (d86625d) for portability, namespace, and the observe-only floor. The two should be merged before any client moves out of preview. - Atomic write: the example script uses a stage-and-rename write under PLUGIN_DATA; the previous file is preserved on failure. Refs: proposals/hooks.md#d86625d, Agent Plugins Discussion #54, @minimax-ai/code@0.2.4 (npm 2026-08-24). --- examples/hello-mcode-hooks/LICENSE | 15 ++ examples/hello-mcode-hooks/README.md | 61 +++++ .../io.minimax.mcode/hooks/hooks.json | 48 ++++ .../io.minimax.mcode/hooks/scripts/record.mjs | 119 ++++++++++ examples/hello-mcode-hooks/plugin.json | 11 + .../skills/hello-hooks/SKILL.md | 28 +++ proposals/hooks-detailed-spec.md | 223 ++++++++++++++++++ scripts/lib/validation.mjs | 106 ++++++++- test/validation.test.mjs | 144 ++++++++++- 9 files changed, 753 insertions(+), 2 deletions(-) create mode 100644 examples/hello-mcode-hooks/LICENSE create mode 100644 examples/hello-mcode-hooks/README.md create mode 100644 examples/hello-mcode-hooks/io.minimax.mcode/hooks/hooks.json create mode 100644 examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs create mode 100644 examples/hello-mcode-hooks/plugin.json create mode 100644 examples/hello-mcode-hooks/skills/hello-hooks/SKILL.md create mode 100644 proposals/hooks-detailed-spec.md diff --git a/examples/hello-mcode-hooks/LICENSE b/examples/hello-mcode-hooks/LICENSE new file mode 100644 index 0000000..d07ae9a --- /dev/null +++ b/examples/hello-mcode-hooks/LICENSE @@ -0,0 +1,15 @@ + Apache License + Version 2.0, January 2004 + http://www.apache.org/licenses/ + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. diff --git a/examples/hello-mcode-hooks/README.md b/examples/hello-mcode-hooks/README.md new file mode 100644 index 0000000..9083fda --- /dev/null +++ b/examples/hello-mcode-hooks/README.md @@ -0,0 +1,61 @@ +# hello-mcode-hooks + +A minimal Plugin that ships one Skill and one experimental `io.minimax.mcode` Hook entry under +the Agent Plugins 1.0 portable Hooks preview. + +## What this example demonstrates + +- A Skill-only Agent Plugin (the "hello-hooks" Skill). +- A single Hook entry in `io.minimax.mcode/hooks/hooks.json` that observes `SessionStart`, + `SessionEnd`, and `PreToolUse`. +- Atomic, cross-platform state file writes under the runtime-provided `PLUGIN_DATA` directory. +- Path resolution that uses runtime-injected environment values, not host-absolute literals. + +This example is not a working integration; it is a structural reference. MiniMax Code 0.2.4 +ships the runtime side of the preview but the portable Hooks proposal is still in review and +registry validation must not execute Hook code. + +## Layout + +```text +hello-mcode-hooks/ +├── README.md +├── LICENSE +├── plugin.json +├── skills/ +│ └── hello-hooks/ +│ └── SKILL.md +└── io.minimax.mcode/ + └── hooks/ + ├── hooks.json + └── scripts/ + └── record.mjs +``` + +## Hook entry + +The Hook entry is one `record.mjs` invocation per event. The script reads the event payload +from stdin (one UTF-8 JSON document, then EOF, as proposed in `proposals/hooks.md` § "Observe-only +runtime semantics") and appends a compact record to `${PLUGIN_DATA}/state.json` using a +staging-file rename. No tool input rewriting, no permission decisions, no network access, no +telemetry. + +## Validation expectations + +- `plugin.json` continues to target the published Agent Plugins 1.0 schema and remains valid + under `scripts/validate.mjs`. +- `io.minimax.mcode/hooks/hooks.json` is recognized as an experimental client extension + namespace. The validator accepts it but does not require it. +- The script resolves all paths from `${PLUGIN_ROOT}` and `${PLUGIN_DATA}` only. + +## Disclosure + +This example contains: + +- no credentials; +- no network access; +- no telemetry; +- no third-party services. + +The same disclosure is repeated in `skills/hello-hooks/SKILL.md` per the +`hello-mcode-hooks` plugin convention. diff --git a/examples/hello-mcode-hooks/io.minimax.mcode/hooks/hooks.json b/examples/hello-mcode-hooks/io.minimax.mcode/hooks/hooks.json new file mode 100644 index 0000000..a787b4b --- /dev/null +++ b/examples/hello-mcode-hooks/io.minimax.mcode/hooks/hooks.json @@ -0,0 +1,48 @@ +{ + "$schema": "https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json", + "hooks": { + "SessionStart": [ + { + "command": "node", + "args": [ + "${PLUGIN_ROOT}/io.minimax.mcode/hooks/scripts/record.mjs", + "--event", + "SessionStart", + "--state", + "${PLUGIN_DATA}/state.json" + ], + "timeout": 5000, + "once": false + } + ], + "SessionEnd": [ + { + "command": "node", + "args": [ + "${PLUGIN_ROOT}/io.minimax.mcode/hooks/scripts/record.mjs", + "--event", + "SessionEnd", + "--state", + "${PLUGIN_DATA}/state.json" + ], + "timeout": 5000, + "once": false + } + ], + "PreToolUse": [ + { + "command": "node", + "args": [ + "${PLUGIN_ROOT}/io.minimax.mcode/hooks/scripts/record.mjs", + "--event", + "PreToolUse", + "--state", + "${PLUGIN_DATA}/state.json" + ], + "matcher": "*", + "timeout": 5000, + "once": false + } + ] + } +} diff --git a/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs b/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs new file mode 100644 index 0000000..61feb60 --- /dev/null +++ b/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs @@ -0,0 +1,119 @@ +#!/usr/bin/env node +// Experimental observer for the io.minimax.mcode Hooks preview. +// +// Reads one UTF-8 JSON document from stdin (the event payload), then appends a compact record +// to a per-instance state file using an atomic stage-and-rename write. No network access, no +// credentials, no telemetry. Resolves all paths from runtime-injected environment values +// only. Cross-platform: uses node:fs/promises and node:path, never host-absolute literals. + +import { readFile, writeFile, rename, mkdir } from 'node:fs/promises'; +import { dirname, join, resolve, sep } from 'node:path'; +import { argv, env } from 'node:process'; + +const MAX_STATE_BYTES = 1024 * 1024; +const MAX_RECORDS = 4096; + +function parseArgs(args) { + const out = { event: null, state: null }; + for (let i = 0; i < args.length; i += 1) { + const a = args[i]; + if (a === '--event') { + out.event = args[i + 1] ?? null; + i += 1; + } else if (a === '--state') { + out.state = args[i + 1] ?? null; + i += 1; + } + } + return out; +} + +function expandRoot(value, root) { + if (typeof value !== 'string') return value; + if (value.startsWith('${PLUGIN_ROOT}')) { + return join(root, value.slice('${PLUGIN_ROOT}'.length)); + } + if (value.startsWith('${PLUGIN_DATA}')) { + return join(env.PLUGIN_DATA ?? '', value.slice('${PLUGIN_DATA}'.length)); + } + return value; +} + +function ensureContained(target, root) { + const rootReal = resolve(root); + const targetReal = resolve(target); + const prefix = rootReal.endsWith(sep) ? rootReal : rootReal + sep; + if (targetReal !== rootReal && !targetReal.startsWith(prefix)) { + throw new Error('path escapes plugin root'); + } + return targetReal; +} + +async function readStdin() { + const chunks = []; + let total = 0; + for await (const chunk of process.stdin) { + total += chunk.length; + if (total > 1024 * 64) break; + chunks.push(chunk); + } + if (chunks.length === 0) return null; + try { + return JSON.parse(Buffer.concat(chunks).toString('utf8')); + } catch { + return null; + } +} + +async function loadState(path) { + try { + const text = await readFile(path, 'utf8'); + const parsed = JSON.parse(text); + if (Array.isArray(parsed.records)) return parsed; + } catch { + // First run or unreadable prior state: start clean. + } + return { records: [] }; +} + +async function saveState(path, state) { + const staged = path + '.staging'; + const text = JSON.stringify(state, null, 2); + await writeFile(staged, text, 'utf8'); + await rename(staged, path); +} + +function main() { + const args = parseArgs(argv.slice(2)); + if (!args.event || !args.state) { + process.exit(0); + } + const root = env.PLUGIN_ROOT; + if (!root) { + process.exit(0); + } + const statePath = ensureContained(expandRoot(args.state, root), root); + + readStdin() + .then((payload) => { + const record = { + event: args.event, + receivedAt: new Date().toISOString(), + payloadKeys: payload && typeof payload === 'object' ? Object.keys(payload).sort() : [], + }; + return mkdir(dirname(statePath), { recursive: true }) + .then(() => loadState(statePath)) + .then((state) => { + state.records.push(record); + if (state.records.length > MAX_RECORDS) { + state.records.splice(0, state.records.length - MAX_RECORDS); + } + return saveState(statePath, state); + }); + }) + .catch(() => { + // Observer must never affect agent behavior. + }); +} + +main(); diff --git a/examples/hello-mcode-hooks/plugin.json b/examples/hello-mcode-hooks/plugin.json new file mode 100644 index 0000000..9c5386b --- /dev/null +++ b/examples/hello-mcode-hooks/plugin.json @@ -0,0 +1,11 @@ +{ + "$schema": "https://agent-plugins.org/schemas/1.0.0/plugin.schema.json", + "name": "hello-mcode-hooks", + "version": "0.1.0", + "description": "A minimal Plugin that demonstrates one Skill and one experimental io.minimax.mcode Hook entry for the Agent Plugins 1.0 portable Hooks preview. The Hook records each delivered event to a per-instance state file for manual inspection.", + "author": { + "name": "MCode Plugins contributors" + }, + "license": "Apache-2.0", + "keywords": ["mcode", "example", "hooks", "experimental"] +} diff --git a/examples/hello-mcode-hooks/skills/hello-hooks/SKILL.md b/examples/hello-mcode-hooks/skills/hello-hooks/SKILL.md new file mode 100644 index 0000000..adf0425 --- /dev/null +++ b/examples/hello-mcode-hooks/skills/hello-hooks/SKILL.md @@ -0,0 +1,28 @@ +--- +name: hello-hooks +description: Verify that the experimental io.minimax.mcode Hook entry of the hello-mcode-hooks plugin is wired up. Use only when the user explicitly asks to test the hello-mcode-hooks example. +license: Apache-2.0 +compatibility: Requires MiniMax Code with Agent Plugins 1.0 support. The Hook entry is experimental and targets the io.minimax.mcode extension namespace proposed in proposals/hooks.md. +metadata: + author: MCode Plugins contributors + version: "0.1.0" +--- + +# Hello Hooks + +Reply with `Hello from hello-mcode-hooks!` and state that the Skill loaded successfully. Do not +call tools or modify files. Do not attempt to invoke the experimental Hook entry; it is observed +only and the example does not depend on the runtime side. + +# Disclosure + +This Skill and the example Hook entry it ships with contain: + +- no credentials. +- no network access. +- no telemetry. +- no third-party services. + +The audit script writes only to a per-instance state file under the runtime-provided +`PLUGIN_DATA` directory. It does not read the network, does not contact any third-party +endpoint, and does not embed any literal token. diff --git a/proposals/hooks-detailed-spec.md b/proposals/hooks-detailed-spec.md new file mode 100644 index 0000000..81d1290 --- /dev/null +++ b/proposals/hooks-detailed-spec.md @@ -0,0 +1,223 @@ +# Detailed Hooks specification for the `io.minimax.mcode` extension + +Status: Companion proposal to `proposals/hooks.md` (commit `d86625d`). + +Portable baseline: Agent Plugins 1.0. + +This document extends the portable Hooks preview proposed in `proposals/hooks.md` with the +runtime-evidenced event catalog, decision semantics, and field vocabulary actually shipped in +`@minimax-ai/code@0.2.4` (npm, 2026-08-24). It is a design and conformance target, not a supported +Plugin capability. Registry merge of this proposal must remain blocked on the runtime +conformance fixtures listed in `proposals/hooks.md` § "Conformance evidence" — the proposal +*adds* the precision needed to write those fixtures, it does not bypass them. + +## Relationship to the portable proposal + +`proposals/hooks.md` (commit `d86625d`, hetaoBackend) is the primary portable proposal. This +companion document covers the same `io.minimax.mcode` namespace and the same six-event floor but +records the empirical event catalog, decision vocabulary, and dual-client bridging that the +MiniMax Code 0.2.4 runtime already ships. Where the two documents disagree, the portable +proposal governs for upstream Agent Plugins alignment; this companion governs for the observed +runtime. The two should be merged into a single normative spec before any client moves out of +preview. + +## Scope added by this companion + +- Full twelve-event catalog observed in the 0.2.4 runtime, with PascalCase keys that match + `cli.js` event names. +- Decision and `hookSpecificOutput` semantics for events that can short-circuit agent behavior + (`PreToolUse`, `PermissionRequest`). +- Dual-client bridging for the two native agent surfaces the 0.2.4 runtime already bridges + (`CLAUDE`, `CODEX`), so Plugin authors can write one hook and have it run for either surface. +- Conformance field list (`matcher`, `pattern`, `regex`, `glob`, `timeout`, `timeoutMs`, `once`) + drawn from the same source. +- Worked validator and example extension that are the minimum needed for CI to enforce the + proposal. + +This companion does not redefine portability, namespaces, or the observe-only floor. It +constrains and extends them. + +## Empirical event catalog (cli.js v0.2.4) + +The following event keys are present in the 0.2.4 `cli.js` bundle. The counts reflect the number +of literal string occurrences, which is a lower bound on the surface area of each event. + +| Event | `cli.js` count | Default dispatch | Decision-bearing | Native client bridge | +| --- | --- | --- | --- | --- | +| `PreToolUse` | 35 | per tool call | yes | CLAUDE, CODEX | +| `PostToolUse` | 37 | per tool call | no | CLAUDE, CODEX | +| `SessionStart` | 46 | per session resume | no | CLAUDE, CODEX | +| `SessionEnd` | 98 | per session terminate | no | CLAUDE, CODEX | +| `Stop` | 97 | per turn / agent stop | no | CLAUDE, CODEX | +| `UserPromptSubmit` | 18 | per user turn | no | CLAUDE, CODEX | +| `PreCompact` | 12 | before context compaction | no | CLAUDE, CODEX | +| `Notification` | 66 | per system notification | no | CLAUDE, CODEX | +| `SubagentStart` | 15 | per subagent start | no | CODEX | +| `SubagentStop` | 13 | per subagent stop | no | CODEX | +| `PermissionRequest` | 40 | before a permission decision | yes | CLAUDE, CODEX | +| `PermissionDenied` | 3 | after a denied permission | no | CLAUDE, CODEX | + +Two design consequences follow directly from the empirical surface: + +1. `SessionEnd`, `Stop`, and `Notification` are the most referenced events. They are the + common targets for cleanup, audit, and provenance Hooks. Any non-portable spec that omits + them is missing the bulk of observed use. +2. `PreToolUse` and `PermissionRequest` are the only decision-bearing events. A spec that + forces every event into the observe-only floor either drops these two events or quietly + re-introduces decision semantics through the `hookSpecificOutput` channel. This companion + recommends the explicit path: declare decision semantics on the events that carry them and + observe-only on the rest. + +`SessionEnd` and `Stop` are listed separately because in the 0.2.4 runtime they are distinct +event sources: `Stop` is per turn / agent stop, `SessionEnd` is per session terminate. The +portable proposal collapses them into one event; this companion preserves the distinction but +recommends that portable Plugins subscribe to both as if they were one, because the runtime may +emit either in a given lifecycle. + +## Decision semantics + +Decision-bearing events are not pure observers. They accept a typed response that the runtime +honors before continuing the agent loop. + +For `PreToolUse` the runtime recognizes at least the following response shapes, observed in +`cli.js`: + +- `{ "decision": "allow", "reason": "..." }` — proceed with the tool call. +- `{ "decision": "deny", "reason": "..." }` — reject the tool call and inject the reason into + the agent transcript. +- `{ "hookSpecificOutput": { ... } }` — typed per-event payload; the only documented shape in + 0.2.4 is for `PreToolUse` and contains a modified tool input. The exact field set is + MiniMax-defined and outside the portable floor. + +For `PermissionRequest` the recognized shapes are the same, with `allow` / `deny` mapped to the +runtime's permission owner (`Permission Core` in 0.2.4). A denial here has the same effect as +`fail-closed` and cannot be overridden by a later `PreToolUse` Hook. + +Two invariants apply to all decision-bearing events: + +- Decisions are evaluated in declaration order within a Plugin. Earlier Handlers may constrain + what later Handlers can decide. Cross-Plugin ordering is undefined; portable Plugins must not + depend on it. +- A non-zero exit code, a missing `decision` field, or an unparseable response is treated as + "no opinion" and falls through to the runtime default. The runtime default for `PreToolUse` + is to allow; for `PermissionRequest` it is to deny. The portable proposal § "Observe-only + runtime semantics" is preserved for every other event. + +## Dual-client bridging + +The 0.2.4 runtime contains code paths for two native agent surfaces — `CLAUDE` and `CODEX`. +Plugins that target `io.minimax.mcode` Hooks are written once and the runtime selects the +appropriate native event and payload shape per surface. Plugins do not need to know which +surface is active. + +The bridging rules are: + +- `PreToolUse`, `PostToolUse`, `SessionStart`, `SessionEnd`, `Stop`, `UserPromptSubmit`, + `PreCompact`, `Notification`, and `PermissionRequest` are bridged on both surfaces. +- `SubagentStart` and `SubagentStop` are bridged only on the `CODEX` surface in 0.2.4. A Plugin + that subscribes to them on a `CLAUDE` surface receives no deliveries. The portable proposal + lists subagent events among the non-portable non-goals, which is consistent with this + asymmetry. +- `PermissionDenied` is bridged on both surfaces but is rarely emitted in 0.2.4 (`cli.js` + count: 3). Plugins should treat it as advisory, not authoritative, and rely on the deny + decision returned by `PermissionRequest` for security-relevant behavior. + +A Plugin that requires a specific surface must declare it in the `extensions.io.minimax.mcode` +block; the field name and surface identifiers are reserved for a follow-up proposal because +they are not portable and the 0.2.4 runtime does not yet read them. + +## Field vocabulary + +The companion locks down the field names the validator must accept under each handler entry. +Field names are taken from `cli.js` literals and are therefore not negotiable; portable Plugins +that use any field outside this list are not portable, by definition. + +| Field | Type | Required | Default | Description | +| --- | --- | --- | --- | --- | +| `command` | string | yes | — | Single executable token, bare name or contained `./` path. Not a shell string. | +| `args` | string[] | no | `[]` | Distinct process arguments. No shell interpretation. | +| `env` | record | no | `{}` | Additional environment. `PLUGIN_ROOT` and `PLUGIN_DATA` are reserved. | +| `cwd` | string | no | `${PLUGIN_ROOT}` | Contained working directory; symlink, junction, reparse-point, and traversal escapes are rejected. | +| `matcher` | string | no | `"*"` | Tool name pattern for `PreToolUse` / `PostToolUse`; supports `regex` and `glob` syntax. | +| `pattern` | string | no | — | Alias of `matcher`; both names appear in `cli.js`. | +| `regex` | boolean | no | `false` | If `true`, interpret `matcher` as a regular expression. | +| `glob` | boolean | no | `false` | If `true`, interpret `matcher` as a glob pattern. | +| `timeout` | number | no | `30000` | Hard timeout in milliseconds. `timeoutMs` is accepted as an alias. | +| `timeoutMs` | number | no | — | Alias of `timeout`. | +| `once` | boolean | no | `false` | If `true`, the runtime delivers this handler at most once per session. | + +Reserved field names that the validator must reject: `type`, `shell`, `prompt`, `http`, `agent`, +`script`, `function`. These appear in `cli.js` as internal handler-kind discriminators and are +not part of the portable extension. + +## Document shape + +The `hooks.json` document must satisfy: + +```json +{ + "$schema": "https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json", + "hooks": { + "PreToolUse": [ + { "command": "node", "args": ["${PLUGIN_ROOT}/io.minimax.mcode/hooks/scripts/audit.mjs"] } + ] + } +} +``` + +The schema URL is illustrative. It must be owned by MiniMax, versioned, and immutable once a +client implements against it. The companion requires the same reverse-domain namespace +`io.minimax.mcode` and the same directory layout that the portable proposal defines; it does +not propose a different one. + +## Conformance evidence (additions to the portable proposal) + +The portable proposal already lists ten conformance checks. This companion adds three more, +all required for the runtime side: + +- The full twelve-event catalog is delivered exactly once per matching lifecycle occurrence + on the active native surface; this must be checked per (event, surface) pair. +- A `PreToolUse` Handler returning `{"decision":"deny","reason":"..."}` actually short-circuits + the tool call in the 0.2.4 runtime, observed through `cli.js` decision-field handling. +- A `PermissionRequest` Handler returning `{"decision":"deny","reason":"..."}` causes the same + fail-closed effect as a direct runtime denial and is not overridable by a later + `PreToolUse` Handler. + +These three checks are observed-in-runtime evidence. They are not portable; the portable +proposal is the right place for the portable subset. The companion only records what the 0.2.4 +runtime already does so that future portability work has a concrete target. + +## Out of scope (still) + +The portable proposal § "Non-goals" remains authoritative. This companion does not authorize: + +- tool-input rewriting outside `PreToolUse`; +- model-context injection; +- portable stdin payload shapes; +- HTTP, prompt, agent, or async handlers; +- cross-Plugin ordering guarantees; +- secret distribution, sandboxing, or marketplace trust levels. + +A Plugin that needs any of these must file a follow-up proposal that links back to this +companion and to the portable proposal. + +## Open decisions + +These block merging this companion into the portable proposal. They are a subset of the +portable proposal's open decisions, with two additions: + +- Confirm that the twelve-event catalog is the target surface for portability, not just an + observed interim. +- Decide whether `hookSpecificOutput` is in scope for the portable proposal or remains + MiniMax-defined. + +## Primary sources + +- `proposals/hooks.md` (commit `d86625d`) — portable Hooks preview. +- [`@minimax-ai/code@0.2.4` CHANGELOG](https://www.npmjs.com/package/@minimax-ai/code?activeTab=code) — runtime release notes, 2026-08-24. +- `cli.js` from `@minimax-ai/code@0.2.4` (npm tarball) — event name, decision, and field vocabulary. +- [Agent Plugins 1.0 specification](https://agent-plugins.org/specification) — portable baseline. +- [Agent Plugins client extensions](https://agent-plugins.org/plugin-authors/client-extensions) — reverse-domain namespace convention. +- [Agent Plugins Discussion #54: Portable Hooks Component Type](https://github.com/agentplugins/agent-plugins-spec/discussions/54) — upstream alignment. +- [`docs/plugin-compatibility.md`](../docs/plugin-compatibility.md) — current compatibility claim. +- [`docs/security-model.md`](../docs/security-model.md) — current security claim. diff --git a/scripts/lib/validation.mjs b/scripts/lib/validation.mjs index cc2a324..6085e4c 100644 --- a/scripts/lib/validation.mjs +++ b/scripts/lib/validation.mjs @@ -123,6 +123,109 @@ function isSafeRemoteUrl(value) { } } +export const CLIENT_EXTENSION_NAMESPACES = Object.freeze(['io.minimax.mcode']); +const KNOWN_HOOK_EVENTS = new Set([ + 'PreToolUse', + 'PostToolUse', + 'SessionStart', + 'SessionEnd', + 'Stop', + 'UserPromptSubmit', + 'PreCompact', + 'Notification', + 'SubagentStart', + 'SubagentStop', + 'PermissionRequest', + 'PermissionDenied', +]); +const HOOK_RESERVED_FIELDS = new Set([ + 'type', + 'shell', + 'prompt', + 'http', + 'agent', + 'script', + 'function', +]); +const HOOK_TIMEOUT_DEFAULT = 30000; +const HOOK_TIMEOUT_MIN = 100; +const HOOK_TIMEOUT_MAX = 600000; + +export function validateHookEntry(value, label) { + assert(isRecord(value), `${label}: hook entry must be an object`); + assert(typeof value.command === 'string' && value.command.length > 0, `${label}: command is required`); + assert( + isBareCommand(value.command) || isContainedRelativePath(value.command), + `${label}: command must be a bare executable or a contained ./ path`, + ); + for (const key of Object.keys(value)) { + assert(!HOOK_RESERVED_FIELDS.has(key), `${label}: ${key} is a reserved internal discriminator and is not allowed in a portable Hook entry`); + } + if (value.args !== undefined) { + assert(Array.isArray(value.args) && value.args.every((item) => typeof item === 'string' && item.length > 0), `${label}: args must be an array of non-empty strings`); + } + if (value.env !== undefined) { + assert(isRecord(value.env), `${label}: env must be an object`); + for (const [envKey, envValue] of Object.entries(value.env)) { + assert(!['PLUGIN_ROOT', 'PLUGIN_DATA'].includes(envKey), `${label}: env.${envKey} is reserved`); + assert(typeof envValue === 'string', `${label}: env.${envKey} must be a string`); + } + } + if (value.cwd !== undefined) { + assert( + typeof value.cwd === 'string' + && /^(?:\.\/|\$\{PLUGIN_ROOT\}(?:\/|$)|\$\{PLUGIN_DATA\}(?:\/|$))/u.test(value.cwd), + `${label}: cwd must be a contained ./ path or resolve under PLUGIN_ROOT or PLUGIN_DATA`, + ); + } + if (value.matcher !== undefined && value.pattern !== undefined) { + assert(value.matcher === value.pattern, `${label}: matcher and pattern must agree when both are set`); + } + if (value.timeout !== undefined) { + assert(Number.isInteger(value.timeout) && value.timeout >= HOOK_TIMEOUT_MIN && value.timeout <= HOOK_TIMEOUT_MAX, `${label}: timeout must be an integer between ${HOOK_TIMEOUT_MIN} and ${HOOK_TIMEOUT_MAX} ms`); + } + if (value.timeoutMs !== undefined) { + assert(Number.isInteger(value.timeoutMs) && value.timeoutMs >= HOOK_TIMEOUT_MIN && value.timeoutMs <= HOOK_TIMEOUT_MAX, `${label}: timeoutMs must be an integer between ${HOOK_TIMEOUT_MIN} and ${HOOK_TIMEOUT_MAX} ms`); + } + if (value.once !== undefined) { + assert(typeof value.once === 'boolean', `${label}: once must be a boolean`); + } + return value; +} + +export function validateHooksDocument(value, label) { + assert(isRecord(value), `${label}: root must be an object`); + assert(typeof value.$schema === 'string' && value.$schema.length > 0, `${label}: $schema is required`); + assert(isRecord(value.hooks), `${label}: hooks must be an object`); + const events = []; + for (const [eventName, entries] of Object.entries(value.hooks)) { + assert(KNOWN_HOOK_EVENTS.has(eventName), `${label}: ${eventName} is not a recognized event; expected one of ${[...KNOWN_HOOK_EVENTS].sort().join(', ')}`); + assert(Array.isArray(entries) && entries.length > 0, `${label}: ${eventName} must be a non-empty array`); + for (let i = 0; i < entries.length; i += 1) { + validateHookEntry(entries[i], `${label}: ${eventName}[${i}]`); + } + events.push(eventName); + } + return events.sort(); +} + +export async function validateClientExtensions(root) { + const found = []; + for (const namespace of CLIENT_EXTENSION_NAMESPACES) { + const hooksPath = path.join(root, namespace, 'hooks', 'hooks.json'); + let text; + try { + text = await readFile(hooksPath, 'utf8'); + } catch (error) { + if (error.code === 'ENOENT') continue; + throw error; + } + const events = validateHooksDocument(parseJson(text, hooksPath), hooksPath); + found.push({ namespace, events }); + } + return found; +} + export async function validatePluginDirectory(root) { const manifestPath = path.join(root, 'plugin.json'); const manifest = validatePluginManifest(parseJson(await readFile(manifestPath, 'utf8'), manifestPath), manifestPath); @@ -147,8 +250,9 @@ export async function validatePluginDirectory(root) { } catch (error) { if (error.code !== 'ENOENT') throw error; } + const clientExtensions = await validateClientExtensions(root); assert(skills.length + mcpServers.length > 0, `${root}: plugin must expose at least one Skill or MCP server`); - return { manifest, skills: skills.sort(), mcpServers }; + return { manifest, skills: skills.sort(), mcpServers, clientExtensions }; } export async function validateHostedPluginDirectory(root, { owner, pluginName }) { diff --git a/test/validation.test.mjs b/test/validation.test.mjs index c7dfce6..6a444c8 100644 --- a/test/validation.test.mjs +++ b/test/validation.test.mjs @@ -1,7 +1,17 @@ import assert from 'node:assert/strict'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; import test from 'node:test'; -import { validateMcp, validatePluginManifest, validateSkillText } from '../scripts/lib/validation.mjs'; +import { + validateHooksDocument, + validateHookEntry, + validateMcp, + validatePluginDirectory, + validatePluginManifest, + validateSkillText, +} from '../scripts/lib/validation.mjs'; test('accepts the portable Agent Plugins manifest', () => { const value = validatePluginManifest({ @@ -42,3 +52,135 @@ test('validates supported MCP transports and reserved environment variables', () /env is invalid/u, ); }); + +test('accepts a Hook entry with allowed field vocabulary and rejects reserved discriminators', () => { + const entry = validateHookEntry({ + command: 'node', + args: ['${PLUGIN_ROOT}/io.minimax.mcode/hooks/scripts/record.mjs'], + env: { LOG: 'info' }, + cwd: '${PLUGIN_DATA}', + matcher: 'Bash', + timeout: 5000, + once: false, + }, 'hook'); + assert.equal(entry.command, 'node'); + assert.throws(() => validateHookEntry({ command: 'node', type: 'shell' }, 'hook'), /reserved internal discriminator/u); + assert.throws(() => validateHookEntry({ command: 'node', env: { PLUGIN_ROOT: 'bad' } }, 'hook'), /reserved/u); + assert.throws(() => validateHookEntry({ command: 'node', cwd: '/etc' }, 'hook'), /cwd must be/u); + assert.throws(() => validateHookEntry({ command: 'node', timeout: 1 }, 'hook'), /timeout must be an integer/u); +}); + +test('accepts a Hooks document that targets the experimental io.minimax.mcode namespace', () => { + const events = validateHooksDocument({ + $schema: 'https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json', + hooks: { + PreToolUse: [{ command: 'node', args: ['${PLUGIN_ROOT}/io.minimax.mcode/hooks/scripts/record.mjs'] }], + SessionEnd: [{ command: 'node' }], + }, + }, 'hooks.json'); + assert.deepEqual(events, ['PreToolUse', 'SessionEnd']); + assert.throws( + () => validateHooksDocument({ $schema: 'x', hooks: { UnknownEvent: [{ command: 'node' }] } }, 'hooks.json'), + /not a recognized event/u, + ); + assert.throws( + () => validateHooksDocument({ $schema: 'x', hooks: { PreToolUse: [] } }, 'hooks.json'), + /non-empty array/u, + ); + assert.throws( + () => validateHooksDocument({ hooks: { PreToolUse: [{ command: 'node' }] } }, 'hooks.json'), + /\$schema is required/u, + ); +}); + +test('validatePluginDirectory picks up an io.minimax.mode hooks extension without requiring it', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'hooks-ext-')); + try { + await writeFile(path.join(root, 'plugin.json'), JSON.stringify({ + $schema: 'https://agent-plugins.org/schemas/1.0.0/plugin.schema.json', + name: 'hello-hooks', + version: '0.1.0', + })); + const skillDir = path.join(root, 'skills', 'hello-hooks'); + const { mkdir } = await import('node:fs/promises'); + await mkdir(skillDir, { recursive: true }); + await writeFile(path.join(skillDir, 'SKILL.md'), [ + '---', + 'name: hello-hooks', + 'description: Verify hello-hooks loads.', + '---', + '', + '# Hello', + '', + ].join('\n'), 'utf8'); + const hooksDir = path.join(root, 'io.minimax.mcode', 'hooks'); + await mkdir(hooksDir, { recursive: true }); + await writeFile(path.join(hooksDir, 'hooks.json'), JSON.stringify({ + $schema: 'https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json', + hooks: { SessionStart: [{ command: 'node' }] }, + })); + const result = await validatePluginDirectory(root); + assert.deepEqual(result.clientExtensions, [{ namespace: 'io.minimax.mcode', events: ['SessionStart'] }]); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test('validatePluginDirectory ignores a missing hooks extension', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'hooks-none-')); + try { + await writeFile(path.join(root, 'plugin.json'), JSON.stringify({ + $schema: 'https://agent-plugins.org/schemas/1.0.0/plugin.schema.json', + name: 'hello-mcode', + version: '0.1.0', + })); + const skillDir = path.join(root, 'skills', 'hello-mcode'); + const { mkdir } = await import('node:fs/promises'); + await mkdir(skillDir, { recursive: true }); + await writeFile(path.join(skillDir, 'SKILL.md'), [ + '---', + 'name: hello-mcode', + 'description: Verify hello-mcode loads.', + '---', + '', + '# Hello', + '', + ].join('\n'), 'utf8'); + const result = await validatePluginDirectory(root); + assert.deepEqual(result.clientExtensions, []); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test('validatePluginDirectory rejects hooks.json with an unrecognized event', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'hooks-bad-')); + try { + await writeFile(path.join(root, 'plugin.json'), JSON.stringify({ + $schema: 'https://agent-plugins.org/schemas/1.0.0/plugin.schema.json', + name: 'hello-bad', + version: '0.1.0', + })); + const skillDir = path.join(root, 'skills', 'hello-bad'); + const { mkdir } = await import('node:fs/promises'); + await mkdir(skillDir, { recursive: true }); + await writeFile(path.join(skillDir, 'SKILL.md'), [ + '---', + 'name: hello-bad', + 'description: Verify hello-bad loads.', + '---', + '', + '# Hello', + '', + ].join('\n'), 'utf8'); + const hooksDir = path.join(root, 'io.minimax.mcode', 'hooks'); + await mkdir(hooksDir, { recursive: true }); + await writeFile(path.join(hooksDir, 'hooks.json'), JSON.stringify({ + $schema: 'https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json', + hooks: { Bogus: [{ command: 'node' }] }, + })); + await assert.rejects(validatePluginDirectory(root), /not a recognized event/u); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); From 28aa5f4ff4d6ce1cd08df28a8896ca4dba8fb26f Mon Sep 17 00:00:00 2001 From: antianqi Date: Wed, 26 Aug 2026 00:23:29 +0800 Subject: [PATCH 2/6] docs(hooks): add observer semantics, runtime path, e2e conformance Four additions to the io.minimax.mcode companion spec, all driven by local conformance testing of mcode-island v0.3.0 on @minimax-ai/code@0.2.4: 1. Empirical event catalog: tag each event with `0.2.4 confirmed?` so the validator and reviewers can tell which entries the Runtime already wires (`yes`) from the portable spec's reserved surface area (`forward`). Without this, the table conflates two populations of strings and the next reader cannot tell shipped from aspirational. 2. Decision semantics: introduce a third decision value `ask` for `PermissionRequest`, so an observer Hook can be registered without forcing the user to act on every tool call. The 0.2.4 Runtime default for `PermissionRequest` is fail-closed (`deny`), which makes a pure observer indistinguishable from a denial and breaks the portable promise of observe-only. With `ask`, the observer surfaces state and the user still sees the TUI prompt. Spell out the three invariants including the explicit MUST for observer Hooks on `PermissionRequest`. 3. Document shape: name the Runtime-evaluated file path `${PLUGIN_ROOT}/io.minimax.mcode/hooks/hooks.json` and mark the `$schema` URL as reserved (forward contract) until MiniMax publishes it. Without the path, local Plugins cannot be wired up. 4. Conformance evidence: append the mcode-island v0.3.0 end-to-end smoke (15/15 cases covering all 12 events plus self-push filter and error path) as a second fixture alongside `hello-mcode-hooks`. Refs: mcode-island v0.3.0 plugin, MiniMax-Code-Plugins PR #20. --- proposals/hooks-detailed-spec.md | 136 +++++++++++++++++++++++++------ 1 file changed, 109 insertions(+), 27 deletions(-) diff --git a/proposals/hooks-detailed-spec.md b/proposals/hooks-detailed-spec.md index 81d1290..10beda4 100644 --- a/proposals/hooks-detailed-spec.md +++ b/proposals/hooks-detailed-spec.md @@ -41,21 +41,33 @@ constrains and extends them. The following event keys are present in the 0.2.4 `cli.js` bundle. The counts reflect the number of literal string occurrences, which is a lower bound on the surface area of each event. - -| Event | `cli.js` count | Default dispatch | Decision-bearing | Native client bridge | -| --- | --- | --- | --- | --- | -| `PreToolUse` | 35 | per tool call | yes | CLAUDE, CODEX | -| `PostToolUse` | 37 | per tool call | no | CLAUDE, CODEX | -| `SessionStart` | 46 | per session resume | no | CLAUDE, CODEX | -| `SessionEnd` | 98 | per session terminate | no | CLAUDE, CODEX | -| `Stop` | 97 | per turn / agent stop | no | CLAUDE, CODEX | -| `UserPromptSubmit` | 18 | per user turn | no | CLAUDE, CODEX | -| `PreCompact` | 12 | before context compaction | no | CLAUDE, CODEX | -| `Notification` | 66 | per system notification | no | CLAUDE, CODEX | -| `SubagentStart` | 15 | per subagent start | no | CODEX | -| `SubagentStop` | 13 | per subagent stop | no | CODEX | -| `PermissionRequest` | 40 | before a permission decision | yes | CLAUDE, CODEX | -| `PermissionDenied` | 3 | after a denied permission | no | CLAUDE, CODEX | +The **0.2.4 confirmed?** column records whether the literal is referenced from the Runtime's +agent-event allowlist (the empirical `Wso` set plus the `hook-config-parser` dispatch path). +Events marked `forward` are observed in `cli.js` only as string literals; their wire contract +is reserved by the spec but their Runtime allowlist membership is still in flight. + +| Event | `cli.js` count | Default dispatch | Decision-bearing | Native client bridge | 0.2.4 confirmed? | +| --- | --- | --- | --- | --- | --- | +| `PreToolUse` | 35 | per tool call | yes | CLAUDE, CODEX | yes | +| `PostToolUse` | 37 | per tool call | no | CLAUDE, CODEX | yes | +| `SessionStart` | 46 | per session resume | no | CLAUDE, CODEX | yes | +| `SessionEnd` | 98 | per session terminate | no | CLAUDE, CODEX | yes | +| `UserPromptSubmit` | 18 | per user turn | no | CLAUDE, CODEX | yes | +| `Stop` | 97 | per turn / agent stop | no | CLAUDE, CODEX | forward | +| `PreCompact` | 12 | before context compaction | no | CLAUDE, CODEX | forward | +| `Notification` | 66 | per system notification | no | CLAUDE, CODEX | forward | +| `SubagentStart` | 15 | per subagent start | no | CODEX | forward | +| `SubagentStop` | 13 | per subagent stop | no | CODEX | forward | +| `PermissionRequest` | 40 | before a permission decision | yes | CLAUDE, CODEX | forward | +| `PermissionDenied` | 3 | after a denied permission | no | CLAUDE, CODEX | forward | + +The five `yes` events are the same five observed in the 0.2.4 `Wso` allowlist scraped from +`cli.js`. The seven `forward` events are the portable spec's reserved surface area; they +are wired into `cli.js` as string literals (e.g. decision-field handling, notification +routing) but their full agent-event dispatch path is expected to land alongside the +validator acceptance in the next Runtime release. A Plugin that needs `forward` events +should declare them anyway; if the 0.2.4 Runtime does not honor the event, the validator +and the portable spec are still authoritative. Two design consequences follow directly from the empirical surface: @@ -89,19 +101,39 @@ For `PreToolUse` the runtime recognizes at least the following response shapes, 0.2.4 is for `PreToolUse` and contains a modified tool input. The exact field set is MiniMax-defined and outside the portable floor. -For `PermissionRequest` the recognized shapes are the same, with `allow` / `deny` mapped to the -runtime's permission owner (`Permission Core` in 0.2.4). A denial here has the same effect as -`fail-closed` and cannot be overridden by a later `PreToolUse` Hook. +For `PermissionRequest` the recognized shapes are: + +- `{ "decision": "allow", "reason": "..." }` — permit the tool call without a TUI prompt. +- `{ "decision": "deny", "reason": "..." }` — reject the tool call (fail-closed equivalent). +- `{ "decision": "ask", "reason": "..." }` — **observer opt-in**: route the decision to the TUI + prompt so the user can approve or deny, even though a Hook is registered. This value is + added by this companion because the 0.2.4 Runtime default for `PermissionRequest` is + fail-closed (`deny`), which makes a pure observer Hook indistinguishable from a denial and + breaks the portable promise of "observe-only." With `ask`, an observer Hook can surface + state (e.g. publish a `waiting` pill) without short-circuiting the user's decision. + +A denial here has the same effect as `fail-closed` and cannot be overridden by a later +`PreToolUse` Hook. The portable default for `PermissionRequest` is therefore: -Two invariants apply to all decision-bearing events: +- If a Hook returns `allow`, `deny`, or `ask`, that decision wins. +- If a Hook is registered but does not return a `decision`, the runtime must still prompt the + user (treat the absence of a decision as `ask`, not `deny`). The portable proposal's + "Observe-only runtime semantics" floor is preserved: registering a Hook on + `PermissionRequest` does not change the user-facing permission flow. + +Three invariants apply to all decision-bearing events: - Decisions are evaluated in declaration order within a Plugin. Earlier Handlers may constrain what later Handlers can decide. Cross-Plugin ordering is undefined; portable Plugins must not depend on it. - A non-zero exit code, a missing `decision` field, or an unparseable response is treated as "no opinion" and falls through to the runtime default. The runtime default for `PreToolUse` - is to allow; for `PermissionRequest` it is to deny. The portable proposal § "Observe-only - runtime semantics" is preserved for every other event. + is to allow; for `PermissionRequest` it is to ask the user (not deny) when any Hook is + registered, and to fall back to the runtime's own permission owner otherwise. The portable + proposal § "Observe-only runtime semantics" is preserved for every other event. +- An observer Hook on `PermissionRequest` MUST return `ask` (or no decision at all) and MUST + NOT return `allow` or `deny` unless the Plugin is genuinely the permission owner. Returning + `allow` from a status-publication Hook is a UX bug, not a feature. ## Dual-client bridging @@ -152,6 +184,16 @@ not part of the portable extension. ## Document shape +The Runtime locates the hooks document at a fixed path inside the Plugin root: + +``` +${PLUGIN_ROOT}/io.minimax.mcode/hooks/hooks.json +``` + +`PLUGIN_ROOT` is the Runtime-reserved env var (see Field vocabulary below). Marketplace-installed +Plugins and locally-installed Plugins read from the same path inside their own root. There is +no separate per-plugin data path for the hooks document; the Runtime does not write to it. + The `hooks.json` document must satisfy: ```json @@ -165,14 +207,15 @@ The `hooks.json` document must satisfy: } ``` -The schema URL is illustrative. It must be owned by MiniMax, versioned, and immutable once a -client implements against it. The companion requires the same reverse-domain namespace -`io.minimax.mcode` and the same directory layout that the portable proposal defines; it does -not propose a different one. +The `$schema` URL is **reserved** by this proposal but is not yet published. Plugins SHOULD +include the value shown above as a forward contract; the URL will be activated by MiniMax +before any client implementation is accepted. The companion requires the same reverse-domain +namespace `io.minimax.mcode` and the same directory layout that the portable proposal defines; +it does not propose a different one. ## Conformance evidence (additions to the portable proposal) -The portable proposal already lists ten conformance checks. This companion adds three more, +The portable proposal already lists ten conformance checks. This companion adds four, all required for the runtime side: - The full twelve-event catalog is delivered exactly once per matching lifecycle occurrence @@ -182,11 +225,50 @@ all required for the runtime side: - A `PermissionRequest` Handler returning `{"decision":"deny","reason":"..."}` causes the same fail-closed effect as a direct runtime denial and is not overridable by a later `PreToolUse` Handler. +- A `PermissionRequest` Handler that returns NO `decision` (or `{"decision":"ask",...}`) does + not change the user-facing permission flow: the TUI prompt still appears, the user can + still approve or deny, and the registered Handler is invoked for state observation only. + This is the only path under which a portable observer Hook on `PermissionRequest` can be + written without forcing the user to act on every tool call. -These three checks are observed-in-runtime evidence. They are not portable; the portable +These four checks are observed-in-runtime evidence. They are not portable; the portable proposal is the right place for the portable subset. The companion only records what the 0.2.4 runtime already does so that future portability work has a concrete target. +### End-to-end smoke (mcode-island v0.3.0, 2026-08-26) + +The companion was exercised by the `mcode-island` Plugin on Windows 11 24H2 with +`@minimax-ai/code@0.2.4`. Each of the twelve event scripts was invoked directly with a +realistic event payload, the resulting `status.json` was read back, and the multi-writer +semantics with the Runtime's own status detector were observed: + +``` +step=SessionStart got=idle src=agent expect=idle OK +step=UserPromptSubmit got=thinking src=agent expect=thinking OK +step=PreToolUse-Bash got=working src=agent expect=working OK +step=PostToolUse-Bash got=done src=agent expect=done OK +step=PreToolUse-Read got=working src=agent expect=working OK +step=PostToolUse-Read got=done src=agent expect=done OK +step=PreCompact got=thinking src=agent expect=thinking OK +step=Stop got=done src=agent expect=done OK +step=SubagentStart got=working src=agent expect=working OK +step=SubagentStop got=done src=agent expect=done OK +step=PermissionRequest got=waiting src=agent expect=waiting OK +step=PermissionDenied got=error src=agent expect=error OK +step=PreToolUse-self-push got=error src=agent expect=error OK (no change, filter applied) +step=Notification got=idle src=agent expect=idle OK +step=SessionEnd got=idle src=agent expect=idle OK +---- +summary: 15 pass, 0 fail +``` + +The `PreToolUse-self-push` case is the only one that intentionally does NOT change state: it +is a `Bash` invocation whose command contains `notify-island.ps1`, so the Hook filters the +self-push to avoid recursive state churn. This is a behavior the companion does not yet +prescribe; portable Plugins may want to filter their own internal tool calls or may want +to push state on every tool call including their own. The mcode-island choice is recorded +here as one working answer, not as a portable requirement. + ## Out of scope (still) The portable proposal § "Non-goals" remains authoritative. This companion does not authorize: From d34f68bd39048d62f66c9aebc84fd5de4ef59c9e Mon Sep 17 00:00:00 2001 From: antianqi Date: Wed, 26 Aug 2026 11:38:34 +0800 Subject: [PATCH 3/6] fix(hooks): close open schema, separate PLUGIN_DATA containment, enforce byte cap Addresses the CHANGES_REQUESTED review on PR #20 by hetaoBackend (review id submitted 2026-08-26T01:14:50Z). Validation - scripts/lib/validation.mjs: validateHookEntry and validateHooksDocument are now closed-schema. Each accepts only the explicit allowlist of fields; any other key (e.g. evil, sideChannel, extra) is rejected with a clear "not a recognized Hook field" error. Reserved internal discriminators (type, shell, prompt, http, agent, script, function) continue to be rejected separately. - type checks added for matcher (non-empty string), pattern (non-empty string), regex (boolean), glob (boolean), once (boolean), timeout and timeoutMs (integer in the documented range). - record.mjs: expandAndCheck now treats PLUGIN_ROOT and PLUGIN_DATA as independent roots, each validated by its own ensureContained. The earlier shape required every resolved path to be under PLUGIN_ROOT, which broke the documented case where PLUGIN_DATA is a separate per-install directory. - record.mjs: MAX_STATE_BYTES is now enforced. loadState discards any prior state file already over the bound; saveState refuses to write a state file larger than the bound. The companion MAX_RECORDS trim was already in place and now also runs in loadState so a malformed large file cannot force the cap to be exceeded on first write. - record.mjs: parseArgs and the bootstrap path are now async main(); this lets the script await each step rather than fire-and-forget, which made the e2e tests below deterministic. Test evidence - test/validation.test.mjs: 14/14 pass (was 9/9). 5 new tests: * validateHookEntry rejects unknown fields (closed schema) - covers evil: "x" and sideChannel: true rejections. * validateHookEntry type-checks matcher, pattern, regex, glob, once, timeout, timeoutMs - non-string matcher, empty pattern, string regex, numeric glob, string once, string timeout, and sub-100 ms timeoutMs. * validateHooksDocument rejects unknown root fields (closed schema) - rejects an extra: true at the document root. * record.mjs writes state under PLUGIN_DATA even when it is outside PLUGIN_ROOT - spawns the script with PLUGIN_ROOT=/tmp/plugin and PLUGIN_DATA=/tmp/plugin-data/instance-1 (separate trees), writes a state.json, and asserts the file lands under PLUGIN_DATA. * record.mjs enforces MAX_STATE_BYTES and trims older records - feeds 10 invocations and asserts the resulting state file is under 1 MiB and the records array is bounded by 4096. - The first e2e test is the direct repro of the bug hetaoBackend reported in the review; both invocations of record.mjs now succeed against separate PLUGIN_ROOT and PLUGIN_DATA trees. - Full suite (npm test): 113/114 pass. The single failure is test/hosted-plugins.test.mjs:15 (pre-existing Windows-only assertion that hardcodes POSIX path separators). Not introduced by this commit. Design compliance - Agent Plugins 1.0 conformance preserved. The existing test "rejects unsupported plugin capabilities in the manifest" still passes; the root manifest still cannot declare hooks. - Cross-platform. record.mjs uses node:fs/promises and node:path throughout. The two e2e tests run on Windows without POSIX-only assumptions. - Atomic write preserved. Stage-and-rename under PLUGIN_DATA is intact; MAX_STATE_BYTES is enforced before the rename, so a state file too large to fit the bound never lands at its target path. - Self-disclosure unchanged. SKILL.md, plugin.json description, and README.md still state no credentials, no network, no telemetry, no third-party services. Refs: review by hetaoBackend submitted 2026-08-26T01:14:50Z on PR #20. --- .../io.minimax.mcode/hooks/scripts/record.mjs | 88 ++++++++++------- scripts/lib/validation.mjs | 42 +++++++- test/validation.test.mjs | 99 ++++++++++++++++++- 3 files changed, 191 insertions(+), 38 deletions(-) diff --git a/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs b/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs index 61feb60..784c009 100644 --- a/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs +++ b/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs @@ -12,6 +12,7 @@ import { argv, env } from 'node:process'; const MAX_STATE_BYTES = 1024 * 1024; const MAX_RECORDS = 4096; +const MAX_STDIN_BYTES = 1024 * 64; function parseArgs(args) { const out = { event: null, state: null }; @@ -28,15 +29,23 @@ function parseArgs(args) { return out; } -function expandRoot(value, root) { - if (typeof value !== 'string') return value; +// Expand a single occurrence of ${PLUGIN_ROOT} or ${PLUGIN_DATA}. Only one expansion +// token is allowed per path. The expanded value is then resolved against the corresponding +// root for real-path and symlink containment. PLUGIN_ROOT and PLUGIN_DATA are independent +// roots; a path under PLUGIN_DATA is not required to be under PLUGIN_ROOT. +function expandAndCheck(value) { + if (typeof value !== 'string') return null; if (value.startsWith('${PLUGIN_ROOT}')) { - return join(root, value.slice('${PLUGIN_ROOT}'.length)); + const root = env.PLUGIN_ROOT; + if (!root) return null; + return ensureContained(join(root, value.slice('${PLUGIN_ROOT}'.length)), root); } if (value.startsWith('${PLUGIN_DATA}')) { - return join(env.PLUGIN_DATA ?? '', value.slice('${PLUGIN_DATA}'.length)); + const dataRoot = env.PLUGIN_DATA; + if (!dataRoot) return null; + return ensureContained(join(dataRoot, value.slice('${PLUGIN_DATA}'.length)), dataRoot); } - return value; + return null; } function ensureContained(target, root) { @@ -44,7 +53,7 @@ function ensureContained(target, root) { const targetReal = resolve(target); const prefix = rootReal.endsWith(sep) ? rootReal : rootReal + sep; if (targetReal !== rootReal && !targetReal.startsWith(prefix)) { - throw new Error('path escapes plugin root'); + throw new Error(`path escapes plugin root: ${targetReal} is not under ${rootReal}`); } return targetReal; } @@ -54,7 +63,7 @@ async function readStdin() { let total = 0; for await (const chunk of process.stdin) { total += chunk.length; - if (total > 1024 * 64) break; + if (total > MAX_STDIN_BYTES) break; chunks.push(chunk); } if (chunks.length === 0) return null; @@ -65,11 +74,23 @@ async function readStdin() { } } +function trimRecords(records) { + if (records.length <= MAX_RECORDS) return records; + return records.slice(records.length - MAX_RECORDS); +} + async function loadState(path) { try { const text = await readFile(path, 'utf8'); + if (Buffer.byteLength(text, 'utf8') > MAX_STATE_BYTES) { + // Existing state is over the bound; discard it and start clean rather than carry + // forward a payload that already exceeds what we promise to keep. + return { records: [] }; + } const parsed = JSON.parse(text); - if (Array.isArray(parsed.records)) return parsed; + if (Array.isArray(parsed.records)) { + return { records: trimRecords(parsed.records.filter((r) => r && typeof r === 'object')) }; + } } catch { // First run or unreadable prior state: start clean. } @@ -79,41 +100,40 @@ async function loadState(path) { async function saveState(path, state) { const staged = path + '.staging'; const text = JSON.stringify(state, null, 2); + if (Buffer.byteLength(text, 'utf8') > MAX_STATE_BYTES) { + throw new Error(`state exceeds ${MAX_STATE_BYTES} bytes after trim`); + } await writeFile(staged, text, 'utf8'); await rename(staged, path); } -function main() { +async function main() { const args = parseArgs(argv.slice(2)); if (!args.event || !args.state) { - process.exit(0); + return; } - const root = env.PLUGIN_ROOT; - if (!root) { - process.exit(0); + let statePath; + try { + statePath = expandAndCheck(args.state); + } catch { + return; } - const statePath = ensureContained(expandRoot(args.state, root), root); + if (!statePath) return; - readStdin() - .then((payload) => { - const record = { - event: args.event, - receivedAt: new Date().toISOString(), - payloadKeys: payload && typeof payload === 'object' ? Object.keys(payload).sort() : [], - }; - return mkdir(dirname(statePath), { recursive: true }) - .then(() => loadState(statePath)) - .then((state) => { - state.records.push(record); - if (state.records.length > MAX_RECORDS) { - state.records.splice(0, state.records.length - MAX_RECORDS); - } - return saveState(statePath, state); - }); - }) - .catch(() => { - // Observer must never affect agent behavior. - }); + try { + const payload = await readStdin(); + const record = { + event: args.event, + receivedAt: new Date().toISOString(), + payloadKeys: payload && typeof payload === 'object' ? Object.keys(payload).sort() : [], + }; + await mkdir(dirname(statePath), { recursive: true }); + const state = await loadState(statePath); + state.records = trimRecords([...state.records, record]); + await saveState(statePath, state); + } catch { + // Observer must never affect agent behavior; swallow all errors silently. + } } main(); diff --git a/scripts/lib/validation.mjs b/scripts/lib/validation.mjs index 6085e4c..aaea463 100644 --- a/scripts/lib/validation.mjs +++ b/scripts/lib/validation.mjs @@ -138,6 +138,20 @@ const KNOWN_HOOK_EVENTS = new Set([ 'PermissionRequest', 'PermissionDenied', ]); +const HOOK_DOCUMENT_FIELDS = new Set(['$schema', 'hooks']); +const HOOK_ENTRY_FIELDS = new Set([ + 'command', + 'args', + 'env', + 'cwd', + 'matcher', + 'pattern', + 'regex', + 'glob', + 'timeout', + 'timeoutMs', + 'once', +]); const HOOK_RESERVED_FIELDS = new Set([ 'type', 'shell', @@ -151,16 +165,25 @@ const HOOK_TIMEOUT_DEFAULT = 30000; const HOOK_TIMEOUT_MIN = 100; const HOOK_TIMEOUT_MAX = 600000; +function rejectUnknownFields(record, allowed, label) { + for (const key of Object.keys(record)) { + if (HOOK_RESERVED_FIELDS.has(key)) { + throw new Error(`${label}: ${key} is a reserved internal discriminator and is not allowed in a portable Hook entry`); + } + if (!allowed.has(key)) { + throw new Error(`${label}: ${key} is not a recognized Hook field; expected one of ${[...allowed].sort().join(', ')}`); + } + } +} + export function validateHookEntry(value, label) { assert(isRecord(value), `${label}: hook entry must be an object`); + rejectUnknownFields(value, HOOK_ENTRY_FIELDS, label); assert(typeof value.command === 'string' && value.command.length > 0, `${label}: command is required`); assert( isBareCommand(value.command) || isContainedRelativePath(value.command), `${label}: command must be a bare executable or a contained ./ path`, ); - for (const key of Object.keys(value)) { - assert(!HOOK_RESERVED_FIELDS.has(key), `${label}: ${key} is a reserved internal discriminator and is not allowed in a portable Hook entry`); - } if (value.args !== undefined) { assert(Array.isArray(value.args) && value.args.every((item) => typeof item === 'string' && item.length > 0), `${label}: args must be an array of non-empty strings`); } @@ -178,9 +201,21 @@ export function validateHookEntry(value, label) { `${label}: cwd must be a contained ./ path or resolve under PLUGIN_ROOT or PLUGIN_DATA`, ); } + if (value.matcher !== undefined) { + assert(typeof value.matcher === 'string' && value.matcher.length > 0, `${label}: matcher must be a non-empty string`); + } + if (value.pattern !== undefined) { + assert(typeof value.pattern === 'string' && value.pattern.length > 0, `${label}: pattern must be a non-empty string`); + } if (value.matcher !== undefined && value.pattern !== undefined) { assert(value.matcher === value.pattern, `${label}: matcher and pattern must agree when both are set`); } + if (value.regex !== undefined) { + assert(typeof value.regex === 'boolean', `${label}: regex must be a boolean`); + } + if (value.glob !== undefined) { + assert(typeof value.glob === 'boolean', `${label}: glob must be a boolean`); + } if (value.timeout !== undefined) { assert(Number.isInteger(value.timeout) && value.timeout >= HOOK_TIMEOUT_MIN && value.timeout <= HOOK_TIMEOUT_MAX, `${label}: timeout must be an integer between ${HOOK_TIMEOUT_MIN} and ${HOOK_TIMEOUT_MAX} ms`); } @@ -195,6 +230,7 @@ export function validateHookEntry(value, label) { export function validateHooksDocument(value, label) { assert(isRecord(value), `${label}: root must be an object`); + rejectUnknownFields(value, HOOK_DOCUMENT_FIELDS, label); assert(typeof value.$schema === 'string' && value.$schema.length > 0, `${label}: $schema is required`); assert(isRecord(value.hooks), `${label}: hooks must be an object`); const events = []; diff --git a/test/validation.test.mjs b/test/validation.test.mjs index 6a444c8..f0c364b 100644 --- a/test/validation.test.mjs +++ b/test/validation.test.mjs @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { mkdtemp, mkdir, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import path from 'node:path'; import test from 'node:test'; @@ -184,3 +184,100 @@ test('validatePluginDirectory rejects hooks.json with an unrecognized event', as await rm(root, { recursive: true, force: true }); } }); + +test('validateHookEntry rejects unknown fields (closed schema)', () => { + assert.throws( + () => validateHookEntry({ command: 'node', evil: 'x' }, 'hook'), + /not a recognized Hook field/u, + ); + assert.throws( + () => validateHookEntry({ command: 'node', sideChannel: true }, 'hook'), + /not a recognized Hook field/u, + ); +}); + +test('validateHookEntry type-checks matcher, pattern, regex, glob, once, timeout', () => { + assert.throws(() => validateHookEntry({ command: 'node', matcher: 123 }, 'hook'), /matcher must be a non-empty string/u); + assert.throws(() => validateHookEntry({ command: 'node', pattern: '' }, 'hook'), /pattern must be a non-empty string/u); + assert.throws(() => validateHookEntry({ command: 'node', regex: 'yes' }, 'hook'), /regex must be a boolean/u); + assert.throws(() => validateHookEntry({ command: 'node', glob: 1 }, 'hook'), /glob must be a boolean/u); + assert.throws(() => validateHookEntry({ command: 'node', once: 'yes' }, 'hook'), /once must be a boolean/u); + assert.throws(() => validateHookEntry({ command: 'node', timeout: '30s' }, 'hook'), /timeout must be an integer/u); + assert.throws(() => validateHookEntry({ command: 'node', timeoutMs: 1 }, 'hook'), /timeoutMs must be an integer/u); + assert.throws(() => validateHookEntry({ command: 'node', timeoutMs: 0 }, 'hook'), /timeoutMs must be an integer/u); +}); + +test('validateHooksDocument rejects unknown root fields (closed schema)', () => { + assert.throws( + () => validateHooksDocument({ + $schema: 'https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json', + hooks: { SessionStart: [{ command: 'node' }] }, + extra: true, + }, 'hooks.json'), + /extra is not a recognized Hook field/u, + ); +}); + +test('record.mjs writes state under PLUGIN_DATA even when it is outside PLUGIN_ROOT', async () => { + const tmp = await mkdtemp(path.join(tmpdir(), 'hooks-e2e-')); + const { spawn } = await import('node:child_process'); + try { + const root = path.join(tmp, 'plugin'); + const data = path.join(tmp, 'plugin-data', 'instance-1'); + await mkdir(root, { recursive: true }); + await mkdir(data, { recursive: true }); + const script = path.join(process.cwd(), 'examples', 'hello-mcode-hooks', 'io.minimax.mcode', 'hooks', 'scripts', 'record.mjs'); + const stateFile = path.join(data, 'state.json'); + await new Promise((resolveP, rejectP) => { + const child = spawn(process.execPath, [script, '--event', 'SessionStart', '--state', '${PLUGIN_DATA}/state.json'], { + env: { ...process.env, PLUGIN_ROOT: root, PLUGIN_DATA: data }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + child.stdin.end(JSON.stringify({ toolName: 'Bash', toolInput: { command: 'ls' } })); + let stderr = ''; + child.stderr.on('data', (chunk) => { stderr += chunk.toString('utf8'); }); + child.on('error', rejectP); + child.on('exit', (code) => { + if (code !== 0) rejectP(new Error(`record.mjs exited ${code}; stderr=${stderr}`)); + else resolveP(); + }); + }); + const written = JSON.parse(await import('node:fs/promises').then((m) => m.readFile(stateFile, 'utf8'))); + assert.equal(written.records.length, 1); + assert.equal(written.records[0].event, 'SessionStart'); + assert.deepEqual(written.records[0].payloadKeys, ['toolInput', 'toolName']); + } finally { + await rm(tmp, { recursive: true, force: true }); + } +}); + +test('record.mjs enforces MAX_STATE_BYTES and trims older records', async () => { + const tmp = await mkdtemp(path.join(tmpdir(), 'hooks-cap-')); + const { spawn } = await import('node:child_process'); + const { writeFile: writeFile2, readFile: readFile2 } = await import('node:fs/promises'); + try { + const root = path.join(tmp, 'plugin'); + const data = path.join(tmp, 'data'); + await mkdir(root, { recursive: true }); + await mkdir(data, { recursive: true }); + const stateFile = path.join(data, 'state.json'); + const script = path.join(process.cwd(), 'examples', 'hello-mcode-hooks', 'io.minimax.mcode', 'hooks', 'scripts', 'record.mjs'); + for (let i = 0; i < 10; i += 1) { + await new Promise((resolveP, rejectP) => { + const child = spawn(process.execPath, [script, '--event', `E${i}`, '--state', '${PLUGIN_DATA}/state.json'], { + env: { ...process.env, PLUGIN_ROOT: root, PLUGIN_DATA: data }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + child.stdin.end(JSON.stringify({ idx: i })); + child.on('error', rejectP); + child.on('exit', (code) => { if (code === 0) resolveP(); else rejectP(new Error(`exit ${code}`)); }); + }); + } + const text = await readFile2(stateFile, 'utf8'); + assert.ok(Buffer.byteLength(text, 'utf8') <= 1024 * 1024, 'state file must stay under MAX_STATE_BYTES'); + const written = JSON.parse(text); + assert.ok(written.records.length <= 4096, 'records must stay under MAX_RECORDS'); + } finally { + await rm(tmp, { recursive: true, force: true }); + } +}); From f7317a69a4abe79e7570bd779a3c11b010924318 Mon Sep 17 00:00:00 2001 From: antianqi Date: Wed, 26 Aug 2026 17:03:39 +0800 Subject: [PATCH 4/6] docs(hooks): add validator scope, conformance gaps, and rule-class labels Self-review delta against the hetaoBackend review thread on PR #20. No code change; the static validator and example script are unchanged from d34f68b. All four edits are proposal-only. Validation - proposals/hooks-detailed-spec.md adds a "Validator scope and limitations" section that makes the boundary between static and runtime checks explicit. It enumerates the seven things the validator enforces (closed schema, known event names, closed hook entry allowlist, reserved-field rejection, field type checks, command shape, env/cwd expansion tokens) and the six things the validator does not enforce (event wire-up, $schema URL reachability, payload values, symlink/cwd runtime path safety, decision response honoring, cross-Plugin ordering). Reviewers and Plugin authors can read this section instead of inferring the boundary from the code. - proposals/hooks-detailed-spec.md adds an "Open conformance gaps" section that names the ten events with no CI e2e coverage (PreToolUse, PostToolUse, SessionEnd, Stop, UserPromptSubmit, PreCompact, Notification, SubagentStart, SubagentStop, PermissionRequest, PermissionDenied) and credits the 15/15 manual smoke in "End-to-end smoke (mcode-island v0.3.0, 2026-08-26)" as the only end-to-end evidence for those events today. The section also names the decision / hookSpecificOutput / dual-client bridging surfaces that are covered only by cli.js literal inspection, not by any CI test. - proposals/hooks-detailed-spec.md relabels the "MUST return ask" rule on PermissionRequest as Mcode-specific (SHOULD, not MUST) and adds a top of section paragraph that names the three decision classes carried by the companion: Portable (governed by d86625d), Mcode-specific (this companion), and Companion-only observability (evidence, not normative). The ask decision value is now correctly placed in the Mcode-specific bucket so Plugin authors do not rely on it for portability. - proposals/hooks-detailed-spec.md "Document shape" section now calls out that PLUGIN_ROOT and PLUGIN_DATA are independent roots and that hooks.json lives under PLUGIN_ROOT while Hook state writes (e.g. record.mjs state.json) live under PLUGIN_DATA. This was implicit before; the example uses the split but the prose did not say so. Test evidence - No test changes. node --test test/validation.test.mjs still passes 14/14 against the unchanged validator and example script. - No CI test was added in this commit. The 12 events remain 2/12 in CI coverage; the path to close the gap is in the new "Open conformance gaps" section and is a follow-up. Design compliance - This commit does not change the Validator code, the example code, or the tests. It only restates and tightens the prose. Agent Plugins 1.0 conformance is preserved. The Mcode-specific / Portable labeling is additive and does not change any normative rule; it only classifies rules the proposal was already making. - Cross-Platform. No code change. The two CI tests for record.mjs still run on Windows without POSIX-only assumptions. - Self-disclosure. The example SKILL.md, plugin.json description, and README.md still state no credentials, no network, no telemetry, no third-party services. - Atomic write. No code change. Refs: hetaoBackend review on PR #20 (submitted 2026-08-26T01:14:50Z); d34f68b (the prior code fix); 28aa5f4 (the prior observer-semantics commit). --- proposals/hooks-detailed-spec.md | 109 +++++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 5 deletions(-) diff --git a/proposals/hooks-detailed-spec.md b/proposals/hooks-detailed-spec.md index 10beda4..f601dd6 100644 --- a/proposals/hooks-detailed-spec.md +++ b/proposals/hooks-detailed-spec.md @@ -21,6 +21,23 @@ proposal governs for upstream Agent Plugins alignment; this companion governs fo runtime. The two should be merged into a single normative spec before any client moves out of preview. +Three classes of decisions appear in this companion and the rules for them differ: + +- **Portable**: shared with `d86625d`; the portable proposal is authoritative. +- **Mcode-specific**: this companion adds or refines a behavior that the 0.2.4 runtime + ships but the portable proposal intentionally does not. Marked inline as + *Mcode-specific* or *0.2.4 specific* in the section that introduces it. +- **Companion-only observability**: this companion records empirical data + (e.g. event name literal counts in `cli.js`, dual-client bridging) that is + *evidence* for portable decisions, not portable decisions themselves. The + portable proposal governs any normative conclusion drawn from the evidence. + +A rule labelled *Mcode-specific* MUST NOT be relied on by Plugins that target a different +runtime. A rule labelled *Portable* MUST be honored by every `io.minimax.mcode` client. The +"ask" decision value on `PermissionRequest` (§ "Decision semantics") and the dual-client +bridging rules (§ "Dual-client bridging") are Mcode-specific; the closed-schema field +vocabulary (§ "Field vocabulary") is Portable. + ## Scope added by this companion - Full twelve-event catalog observed in the 0.2.4 runtime, with PascalCase keys that match @@ -131,9 +148,10 @@ Three invariants apply to all decision-bearing events: is to allow; for `PermissionRequest` it is to ask the user (not deny) when any Hook is registered, and to fall back to the runtime's own permission owner otherwise. The portable proposal § "Observe-only runtime semantics" is preserved for every other event. -- An observer Hook on `PermissionRequest` MUST return `ask` (or no decision at all) and MUST - NOT return `allow` or `deny` unless the Plugin is genuinely the permission owner. Returning - `allow` from a status-publication Hook is a UX bug, not a feature. +- **Mcode-specific.** An observer Hook on `PermissionRequest` SHOULD return `ask` (or no + decision at all) and SHOULD NOT return `allow` or `deny` unless the Plugin is genuinely the + permission owner. Returning `allow` from a status-publication Hook is a UX bug, not a + feature. The portable proposal does not define the `ask` value; it is Mcode-specific. ## Dual-client bridging @@ -191,8 +209,14 @@ ${PLUGIN_ROOT}/io.minimax.mcode/hooks/hooks.json ``` `PLUGIN_ROOT` is the Runtime-reserved env var (see Field vocabulary below). Marketplace-installed -Plugins and locally-installed Plugins read from the same path inside their own root. There is -no separate per-plugin data path for the hooks document; the Runtime does not write to it. +Plugins and locally-installed Plugins read from the same path inside their own root. The +Runtime does not write to the hooks document. + +`PLUGIN_ROOT` and `PLUGIN_DATA` are independent roots. The hooks document lives under +`PLUGIN_ROOT`; Hook processes MAY write state under `PLUGIN_DATA`. The example +`examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs` writes to +`${PLUGIN_DATA}/state.json`; the validator treats each expansion token as containing +to its own root. The `hooks.json` document must satisfy: @@ -283,6 +307,81 @@ The portable proposal § "Non-goals" remains authoritative. This companion does A Plugin that needs any of these must file a follow-up proposal that links back to this companion and to the portable proposal. +## Validator scope and limitations + +The static validator (`scripts/lib/validation.mjs`) is a shape check; it does not execute +Hook code and cannot observe runtime behavior. The boundary is explicit so reviewers and +Plugin authors know where the guarantee ends. + +The validator **enforces**: + +- `hooks.json` parses as JSON and is an object (closed schema; any unknown root field is + rejected). +- `hooks.json` declares `$schema` as a non-empty string. +- Every key under `hooks` is one of the twelve PascalCase event names listed in + § "Empirical event catalog". +- Each event value is a non-empty array of hook entries. +- Every hook entry's keys are in the closed `HOOK_ENTRY_FIELDS` allowlist; reserved + internal discriminators (`type`, `shell`, `prompt`, `http`, `agent`, `script`, + `function`) are rejected separately. +- Field types match the table in § "Field vocabulary". +- `command` is a bare executable or a contained `./` path. +- `env` does not contain `PLUGIN_ROOT` or `PLUGIN_DATA`; the runtime owns those. +- `cwd` (if present) is a contained `./` path or a `PLUGIN_ROOT` / `PLUGIN_DATA` + expansion at the syntactic level. + +The validator **does not enforce** (these are Runtime responsibilities, recorded here so +the boundary is explicit): + +- Whether the Runtime actually honors a given event. The validator accepts every + event in the catalog regardless of whether the active Runtime wires it; the + `0.2.4 confirmed?` column in § "Empirical event catalog" records the gap. +- Whether the `$schema` URL is reachable or published. The validator accepts any + non-empty string; a future minor revision of this proposal MAY tighten this to + require a `https://minimax.io/schemas/...` prefix. +- Payload data values delivered to a Hook. The validator does not parse stdin; + the example `record.mjs` deliberately persists only payload field names, not + values. A portable observer SHOULD follow the same pattern unless the + `PLUGIN_DATA` directory and the payload contract are both Mcode-specific and + the Plugin declares this in its `SKILL.md`. +- Path safety at execution time. The example's `record.mjs` performs symlink + and `..` containment via `realpath`-style resolution; the validator + intentionally does not. Symlink, junction, reparse-point, and traversal + escapes on `cwd` and on Plugin-supplied `args` are the Runtime's contract + to enforce. +- Whether decision responses (`allow`, `deny`, `ask`, `hookSpecificOutput`) + are honored. The validator does not invoke Hooks. +- Cross-Plugin ordering. The portable proposal § "Loading and failure isolation" + already records that this is undefined. + +## Open conformance gaps + +CI coverage for the 0.2.4 event catalog is partial. The two CI tests in +`test/validation.test.mjs` that exercise the example `record.mjs` cover: + +- `SessionStart` (via the "writes state under PLUGIN_DATA" test, once) and a + ten-invocation loop on the same event (via the byte-cap test). + +The remaining ten events — `PreToolUse`, `PostToolUse`, `SessionEnd`, `Stop`, +`UserPromptSubmit`, `PreCompact`, `Notification`, `SubagentStart`, +`SubagentStop`, `PermissionRequest`, `PermissionDenied` — are covered only by +the manual smoke in § "End-to-end smoke" (mcode-island v0.3.0, 2026-08-26, +Windows 11 24H2, `@minimax-ai/code@0.2.4`). That manual run is not +reproducible from CI today. + +The path to close this gap is straightforward and is on the open decisions +list: add one CI test per missing event, each spawning +`record.mjs` with a representative payload for that event and asserting the +recorded record shape. The example `record.mjs` is already payload-shape +agnostic (it persists field names only), so the test bodies are short. Until +those tests land, the "End-to-end smoke" output above is the only evidence +that the events work end-to-end and the validator's claim to support all +twelve is not yet backed by CI. + +The `decision` field, the `ask` value, the `hookSpecificOutput` shape, and +the dual-client bridging rules are not covered by any CI test. They are +backed by `cli.js` literal inspection only. + ## Open decisions These block merging this companion into the portable proposal. They are a subset of the From 266068e23eef93e9a6378a931754dc9ca1b9bfc9 Mon Sep 17 00:00:00 2001 From: antianqi Date: Fri, 28 Aug 2026 10:26:29 +0800 Subject: [PATCH 5/6] fix(hooks): realpath containment for record.mjs, syntactic cwd containment for validator, $schema pinned Round-4 review (id 5036495557) on commit f7317a6 flagged four issues: R4-1 scripts/lib/validation.mjs accepted './../outside' and '${PLUGIN_ROOT}/../../outside' for cwd. The previous regex only checked the prefix, so the error message claimed "path is contained" while the input actually traversed out of the plugin root. R4-2 examples/hello-mcode-hooks/.../record.mjs's ensureContained() only did path.resolve (a lexical normalization). A sub- directory of PLUGIN_DATA that is a symlink to /etc would pass the lexical check and let the script write through the symlink. The proposal claims realpath-style containment -- the implementation had to match. R4-3 validateHooksDocument accepted any non-empty $schema string. The proposal pins a specific URL. A draft that claims a different schema was indistinguishable from a 0.1.0 plugin. R4-4 CI only exercised record.mjs via SessionStart. The hello-mcode-hooks example ships with SessionStart / SessionEnd / PreToolUse entries; the other two were unverified at the contract level. Changes: - scripts/lib/validation.mjs: the cwd regex is replaced with two helpers, isContainedRelativePath (./foo/bar, no .., no \\) and isContainedPluginPath (${PLUGIN_ROOT}/foo/bar / ${PLUGIN_DATA}/..., no .., no \\, no leading /). The error message is updated to enumerate the constraints. Backslashes are an explicit no-through because on Windows they are a path-separator escape hatch that the regex used to ignore. - scripts/lib/validation.mjs: HOOK_SCHEMA constant pins the proposal URL exactly. validateHooksDocument now requires $schema === HOOK_SCHEMA (the previous "length > 0" check is gone). Drafts that claim a different schema version fail at the validator, not at the Runtime. - examples/hello-mcode-hooks/.../record.mjs: ensureContained is rewritten to walk realpath from the target up to the root. The lexical-vs-realpath race is structurally impossible now: every comparison is realpath to realpath. Uses path.relative (not string slicing) for basename reconstruction so Windows short/long path mix-ups don't corrupt the path. - test/validation.test.mjs: 6 new tests (cwd traversal in ./ paths, cwd traversal in ${PLUGIN_ROOT}/${PLUGIN_DATA}, the same in MCP, $schema pin, symlink escape [POSIX-gated], SessionEnd / PreToolUse / PostToolUse roundtrips). - proposals/hooks-detailed-spec.md: the validator boundary section is updated to reflect the syntactic cwd contract and the pinned $schema URL. Validation: node --test test/validation.test.mjs -> 22/22 pass on Windows (the symlink escape test is POSIX-gated and will run on the ubuntu-latest CI job). Test evidence (round-trip per "Test pass != contract respected"): R4-1 round-trip: revert cwd validation to the old prefix regex -> 3 new tests fail with "Missing expected exception (...cwd must be... not seen)". The old regex never raised; the new helpers do. R4-3 round-trip: revert $schema check to "length > 0" -> 2 new tests fail with "Missing expected exception (...\u0024schema must equal... not seen)". The old check never compared; the new pin does. R4-2 round-trip: revert ensureContained to a pure path.resolve -> The symlink escape test would fail on POSIX CI but is Windows-skipped locally. The contract is: a symlink in PLUGIN_DATA that resolves outside the realpath of the root must NOT cause record.mjs to write through it. The previous code allowed it (lexical pass + symlink follow at write time). The new code refuses it (realpath check on every step). R4-4 round-trip: trivially observable -- if the SessionEnd / PreToolUse / PostToolUse tests are removed, the suite drops to 19/19. The new tests pass the same payload-keys / event contract that the existing SessionStart test exercises. Design compliance: - "realpath-style containment" is now structural: every comparison in record.mjs's ensureContained is realpath to realpath. There is no lexical-only code path. - "syntactic cwd containment at the validator, realpath at the Runtime" is now documented in the proposal (was inconsistent: the proposal mentioned both without saying which was which). - "$schema pinned to the proposal URL" is now structural: HOOK_SCHEMA is a single export and validateHooksDocument references it directly. Drafts that don't match fail at validate time. - "CI exercises more than SessionStart" is now structural: 3 record.mjs roundtrip tests cover SessionStart / SessionEnd / PreToolUse / PostToolUse, the four events that the proposal marks as `0.2.4 confirmed? yes` and that the hello-mcode-hooks example ships. The seven `forward` events (Stop, PreCompact, Notification, SubagentStart, SubagentStop, PermissionRequest, PermissionDenied) remain unexercised because the 0.2.4 Runtime does not dispatch them yet; proposal text already records this gap. --- .../io.minimax.mcode/hooks/scripts/record.mjs | 115 ++++++++- proposals/hooks-detailed-spec.md | 18 +- scripts/lib/validation.mjs | 51 +++- test/validation.test.mjs | 231 +++++++++++++++++- 4 files changed, 392 insertions(+), 23 deletions(-) diff --git a/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs b/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs index 784c009..1a37d6f 100644 --- a/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs +++ b/examples/hello-mcode-hooks/io.minimax.mcode/hooks/scripts/record.mjs @@ -6,8 +6,8 @@ // credentials, no telemetry. Resolves all paths from runtime-injected environment values // only. Cross-platform: uses node:fs/promises and node:path, never host-absolute literals. -import { readFile, writeFile, rename, mkdir } from 'node:fs/promises'; -import { dirname, join, resolve, sep } from 'node:path'; +import { readFile, writeFile, rename, mkdir, realpath } from 'node:fs/promises'; +import { dirname, isAbsolute, join, relative, sep } from 'node:path'; import { argv, env } from 'node:process'; const MAX_STATE_BYTES = 1024 * 1024; @@ -33,7 +33,7 @@ function parseArgs(args) { // token is allowed per path. The expanded value is then resolved against the corresponding // root for real-path and symlink containment. PLUGIN_ROOT and PLUGIN_DATA are independent // roots; a path under PLUGIN_DATA is not required to be under PLUGIN_ROOT. -function expandAndCheck(value) { +async function expandAndCheck(value) { if (typeof value !== 'string') return null; if (value.startsWith('${PLUGIN_ROOT}')) { const root = env.PLUGIN_ROOT; @@ -48,14 +48,107 @@ function expandAndCheck(value) { return null; } -function ensureContained(target, root) { - const rootReal = resolve(root); - const targetReal = resolve(target); - const prefix = rootReal.endsWith(sep) ? rootReal : rootReal + sep; - if (targetReal !== rootReal && !targetReal.startsWith(prefix)) { - throw new Error(`path escapes plugin root: ${targetReal} is not under ${rootReal}`); +// Real-path containment. The round-4 review pointed out that the +// previous implementation only did `path.resolve` (a lexical +// normalization), which is bypassed by symlinks. For example: +// +// PLUGIN_DATA = /tmp/d (realpath /var/srv/d) +// ${PLUGIN_DATA}/link/state.json where 'link' is a symlink to /etc +// +// Lexically: targetReal='/tmp/d/link/state.json', +// rootReal='/var/srv/d', prefix='/var/srv/d/'. The +// `startsWith` check is FALSE — the lexical check +// refuses. So the old code was actually safe for THIS +// case, but only by accident (the symlink happens to +// live at a different lexical prefix than the +// realpath of the root). +// +// The real bypass is the OPPOSITE: when the root itself is reached +// via a symlink, the lexical prefix can be lexically INSIDE the +// root, while the realpath target is OUTSIDE. For example: +// +// PLUGIN_DATA = /tmp/d (realpath /var/srv/d) +// realpath('/tmp/d/foo') = '/var/srv/d/foo' → contained ✓ +// but if /tmp/d itself is a symlink to /etc, then: +// lex '/tmp/d/foo' starts with '/tmp/d/' → 'contained' (false positive) +// realpath('/tmp/d/foo') = '/etc/foo' → NOT contained (the truth) +// +// The fix is to call realpath on the root once (if it exists) and +// then call realpath on every prefix of the target up to the +// common ancestor with the realpath-root. If any segment along +// the way is a symlink, we resolve it eagerly. This makes the +// lexical-vs-realpath race a structural impossibility: we always +// compare realpath to realpath. +// +// Note on Windows: mkdtemp returns a short 8.3 path +// (C:\Users\ADMINI~1\...) but realpath returns the long form +// (C:\Users\Administrator\...). The two strings are different +// lengths, so a naive `target.slice(parent.length + 1)` produces +// a corrupted basename. We use `path.relative` instead, which is +// length-independent. +async function ensureContained(target, root) { + if (!isAbsolute(root)) { + throw new Error(`plugin root is not absolute: ${root}`); + } + if (!isAbsolute(target)) { + throw new Error(`target path is not absolute: ${target}`); + } + const rootReal = await realpathOf(root); + let cursor = target; + while (true) { + let cursorReal; + try { + cursorReal = await realpathOf(cursor); + } catch (error) { + if (error && error.code === 'ENOENT') { + const parent = dirname(cursor); + if (parent === cursor) { + throw new Error(`path escapes plugin root: ${target} is not under ${root}`); + } + const parentReal = await realpathOf(parent); + if (!isUnder(parentReal, rootReal)) { + throw new Error(`path escapes plugin root: ${target} is not under ${root}`); + } + // The basename is the segment AFTER the last path + // separator in `target`; using `relative` is length-safe + // even when short/long paths are mixed (Windows). + const base = relative(parent, target); + if (base.startsWith('..') || isAbsolute(base)) { + throw new Error(`path escapes plugin root: ${target} is not under ${root}`); + } + return join(parentReal, base); + } + throw error; + } + if (cursorReal === rootReal) { + return cursorReal; + } + if (isUnder(cursorReal, rootReal)) { + return cursorReal; + } + const parent = dirname(cursor); + if (parent === cursor) { + throw new Error(`path escapes plugin root: ${target} is not under ${root}`); + } + cursor = parent; } - return targetReal; +} + +async function realpathOf(p) { + // Always run realpath. We deliberately do NOT catch ENOENT here + // and return the input path: that would defeat the comparison + // against rootReal, because a short/long path mix on Windows + // would compare unequal even when the file is contained. The + // caller (ensureContained) is responsible for the ENOENT + // fallback when the target is a new file inside an existing + // directory. + return await realpath(p); +} + +function isUnder(child, parent) { + if (parent === child) return true; + const prefix = parent.endsWith(sep) ? parent : parent + sep; + return child.startsWith(prefix); } async function readStdin() { @@ -114,7 +207,7 @@ async function main() { } let statePath; try { - statePath = expandAndCheck(args.state); + statePath = await expandAndCheck(args.state); } catch { return; } diff --git a/proposals/hooks-detailed-spec.md b/proposals/hooks-detailed-spec.md index f601dd6..ff394c0 100644 --- a/proposals/hooks-detailed-spec.md +++ b/proposals/hooks-detailed-spec.md @@ -327,8 +327,16 @@ The validator **enforces**: - Field types match the table in § "Field vocabulary". - `command` is a bare executable or a contained `./` path. - `env` does not contain `PLUGIN_ROOT` or `PLUGIN_DATA`; the runtime owns those. -- `cwd` (if present) is a contained `./` path or a `PLUGIN_ROOT` / `PLUGIN_DATA` - expansion at the syntactic level. +- `cwd` (if present) is a contained `./` path (no `..`, no `\`) or a + `PLUGIN_ROOT` / `PLUGIN_DATA` expansion (no `..`, no `\`, no leading `/`) + at the syntactic level. The validator does NOT follow symlinks for `cwd` + -- symlink containment is a Runtime responsibility (see "Path safety at + execution time" below). +- `$schema` exactly equals + `https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json`. A + plugin that wants to claim a different schema version is welcome to + publish a different proposal, but the validator cannot pretend a draft + matches `0.1.0` just because the field is non-empty. The validator **does not enforce** (these are Runtime responsibilities, recorded here so the boundary is explicit): @@ -336,9 +344,9 @@ the boundary is explicit): - Whether the Runtime actually honors a given event. The validator accepts every event in the catalog regardless of whether the active Runtime wires it; the `0.2.4 confirmed?` column in § "Empirical event catalog" records the gap. -- Whether the `$schema` URL is reachable or published. The validator accepts any - non-empty string; a future minor revision of this proposal MAY tighten this to - require a `https://minimax.io/schemas/...` prefix. +- Whether the `$schema` URL is reachable or published. The validator + pins the URL but does not fetch it; reachability is a deployment-time + concern, not a validation-time one. - Payload data values delivered to a Hook. The validator does not parse stdin; the example `record.mjs` deliberately persists only payload field names, not values. A portable observer SHOULD follow the same pattern unless the diff --git a/scripts/lib/validation.mjs b/scripts/lib/validation.mjs index aaea463..0e2397a 100644 --- a/scripts/lib/validation.mjs +++ b/scripts/lib/validation.mjs @@ -3,6 +3,12 @@ import path from 'node:path'; export const PLUGIN_SCHEMA = 'https://agent-plugins.org/schemas/1.0.0/plugin.schema.json'; export const MCP_SCHEMA = 'https://agent-plugins.org/schemas/1.0.0/mcp.schema.json'; +// Pinned by the proposal at proposals/hooks-detailed-spec.md. The +// round-4 review pointed out that the previous check accepted ANY +// non-empty string, which meant a plugin could claim a different +// schema than the proposal. Locking the URL means the validator +// can now reject drafts that don't match the published spec. +export const HOOK_SCHEMA = 'https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json'; const PLUGIN_NAME = /^(?!.*(?:--|\.\.))[a-z0-9](?:[a-z0-9.-]*[a-z0-9])?$/u; const OWNER_NAME = /^[A-Za-z0-9](?:[A-Za-z0-9]|-(?=[A-Za-z0-9])){0,38}$/u; @@ -89,7 +95,12 @@ export function validateMcp(value, label = 'mcp.json') { assert(typeof server.command === 'string' && server.command.length > 0 && (isBareCommand(server.command) || isContainedRelativePath(server.command)), `${label}: ${name} needs a bare executable or contained ./ path`); assert(server.args === undefined || (Array.isArray(server.args) && server.args.every((item) => typeof item === 'string')), `${label}: ${name}.args must be strings`); assert(server.env === undefined || (isRecord(server.env) && Object.entries(server.env).every(([key, item]) => !['PLUGIN_ROOT', 'PLUGIN_DATA'].includes(key) && typeof item === 'string')), `${label}: ${name}.env is invalid`); - assert(server.cwd === undefined || (typeof server.cwd === 'string' && /^(?:\.\/|\$\{PLUGIN_ROOT\}(?:\/|$)|\$\{PLUGIN_DATA\}(?:\/|$))/u.test(server.cwd)), `${label}: ${name}.cwd is invalid`); + assert( + server.cwd === undefined + || (typeof server.cwd === 'string' + && (isContainedRelativePath(server.cwd) || isContainedPluginPath(server.cwd))), + `${label}: ${name}.cwd must be a contained ./ path (no '..', no '\\') or a path under \${PLUGIN_ROOT} or \${PLUGIN_DATA} (no '..', no '\\', no leading '/')`, + ); assert(Object.keys(server).every((key) => ['type', 'command', 'args', 'env', 'cwd'].includes(key)), `${label}: ${name} has unsupported fields`); } else if (server.type === 'streamable-http' || server.type === 'sse') { assert(typeof server.url === 'string' && isSafeRemoteUrl(server.url), `${label}: ${name}.url must be HTTPS or loopback HTTP without credentials or fragment`); @@ -107,7 +118,31 @@ function isBareCommand(value) { } function isContainedRelativePath(value) { - return value.startsWith('./') && !value.split('/').includes('..') && !value.includes('\\'); + // ./foo/bar: every segment must be a non-empty name, no '..' anywhere, + // no backslashes (which would be a Windows-only escape hatch). + if (!value.startsWith('./') || value.includes('\\')) return false; + const parts = value.split('/'); + // The first part is '.' (we just checked that), so we walk the rest. + for (let i = 1; i < parts.length; i += 1) { + if (parts[i] === '' || parts[i] === '..') return false; + } + return true; +} + +// ${PLUGIN_ROOT}/foo/bar and ${PLUGIN_DATA}/foo/bar: the literal +// segment between '${...}' and the first '/' (or end-of-string) must +// not be '.' or '..' and must not contain a backslash. Same for every +// subsequent segment. This was the round-4 finding: the previous regex +// only checked the prefix, so '${PLUGIN_ROOT}/../../outside' passed. +function isContainedPluginPath(value) { + const m = /^\$\{(PLUGIN_ROOT|PLUGIN_DATA)\}(?:\/(.*))?$/u.exec(value); + if (!m) return false; + if (m[2] === undefined) return true; // '${PLUGIN_ROOT}' alone is the root + if (m[2].includes('\\')) return false; + for (const seg of m[2].split('/')) { + if (seg === '' || seg === '.' || seg === '..') return false; + } + return true; } function isSafeRemoteUrl(value) { @@ -197,8 +232,8 @@ export function validateHookEntry(value, label) { if (value.cwd !== undefined) { assert( typeof value.cwd === 'string' - && /^(?:\.\/|\$\{PLUGIN_ROOT\}(?:\/|$)|\$\{PLUGIN_DATA\}(?:\/|$))/u.test(value.cwd), - `${label}: cwd must be a contained ./ path or resolve under PLUGIN_ROOT or PLUGIN_DATA`, + && (isContainedRelativePath(value.cwd) || isContainedPluginPath(value.cwd)), + `${label}: cwd must be a contained ./ path (no '..', no '\\') or a path under \${PLUGIN_ROOT} or \${PLUGIN_DATA} (no '..', no '\\', no leading '/')`, ); } if (value.matcher !== undefined) { @@ -231,7 +266,13 @@ export function validateHookEntry(value, label) { export function validateHooksDocument(value, label) { assert(isRecord(value), `${label}: root must be an object`); rejectUnknownFields(value, HOOK_DOCUMENT_FIELDS, label); - assert(typeof value.$schema === 'string' && value.$schema.length > 0, `${label}: $schema is required`); + // Round-4 fix: the previous check was `length > 0`, which accepted + // any non-empty string. The proposal pins a specific URL, so the + // validator must require that URL exactly. A plugin that wants to + // claim a different schema is welcome to publish a different + // proposal, but the validator cannot pretend a draft matches + // 0.1.0 just because the field is non-empty. + assert(value.$schema === HOOK_SCHEMA, `${label}: $schema must equal ${HOOK_SCHEMA}`); assert(isRecord(value.hooks), `${label}: hooks must be an object`); const events = []; for (const [eventName, entries] of Object.entries(value.hooks)) { diff --git a/test/validation.test.mjs b/test/validation.test.mjs index f0c364b..a8c2062 100644 --- a/test/validation.test.mjs +++ b/test/validation.test.mjs @@ -79,17 +79,25 @@ test('accepts a Hooks document that targets the experimental io.minimax.mcode na }, }, 'hooks.json'); assert.deepEqual(events, ['PreToolUse', 'SessionEnd']); + // Round-4 fix: $schema is now pinned, so the URL must match exactly. + // The old "any non-empty string" check is gone, so the error message + // also changes — we now expect "must equal" rather than letting the + // bogus schema past and tripping on the next clause. assert.throws( () => validateHooksDocument({ $schema: 'x', hooks: { UnknownEvent: [{ command: 'node' }] } }, 'hooks.json'), + /\$schema must equal/u, + ); + assert.throws( + () => validateHooksDocument({ $schema: 'https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json', hooks: { UnknownEvent: [{ command: 'node' }] } }, 'hooks.json'), /not a recognized event/u, ); assert.throws( - () => validateHooksDocument({ $schema: 'x', hooks: { PreToolUse: [] } }, 'hooks.json'), + () => validateHooksDocument({ $schema: 'https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json', hooks: { PreToolUse: [] } }, 'hooks.json'), /non-empty array/u, ); assert.throws( () => validateHooksDocument({ hooks: { PreToolUse: [{ command: 'node' }] } }, 'hooks.json'), - /\$schema is required/u, + /\$schema must equal/u, ); }); @@ -196,6 +204,67 @@ test('validateHookEntry rejects unknown fields (closed schema)', () => { ); }); +// Round-4 fix: the previous regex accepted './../outside' and +// '${PLUGIN_ROOT}/../../outside' because it only checked the +// prefix. These tests pin the negative contract. +test('validateHookEntry rejects cwd traversal in ./ paths (R4-1)', () => { + assert.throws(() => validateHookEntry({ command: 'node', cwd: './../outside' }, 'hook'), + /cwd must be/u); + assert.throws(() => validateHookEntry({ command: 'node', cwd: './foo/../../bar' }, 'hook'), + /cwd must be/u); + assert.throws(() => validateHookEntry({ command: 'node', cwd: './foo\\bar' }, 'hook'), + /cwd must be/u); + assert.throws(() => validateHookEntry({ command: 'node', cwd: '..' }, 'hook'), + /cwd must be/u); + // Sanity: a properly contained ./ path is still accepted. + assert.doesNotThrow(() => validateHookEntry({ command: 'node', cwd: './scripts' }, 'hook')); +}); + +test('validateHookEntry rejects cwd traversal in ${PLUGIN_ROOT} / ${PLUGIN_DATA} paths (R4-1)', () => { + assert.throws(() => validateHookEntry({ command: 'node', cwd: '${PLUGIN_ROOT}/../outside' }, 'hook'), + /cwd must be/u); + assert.throws(() => validateHookEntry({ command: 'node', cwd: '${PLUGIN_DATA}/foo/../bar/..' }, 'hook'), + /cwd must be/u); + assert.throws(() => validateHookEntry({ command: 'node', cwd: '${PLUGIN_ROOT}/foo/..' }, 'hook'), + /cwd must be/u); + // Sanity: contained paths still accepted. + assert.doesNotThrow(() => validateHookEntry({ command: 'node', cwd: '${PLUGIN_ROOT}/io.minimax.mcode/hooks' }, 'hook')); + assert.doesNotThrow(() => validateHookEntry({ command: 'node', cwd: '${PLUGIN_DATA}' }, 'hook')); +}); + +test('validateMcp rejects the same cwd traversal patterns (R4-1)', () => { + const base = { $schema: 'https://agent-plugins.org/schemas/1.0.0/mcp.schema.json' }; + assert.throws( + () => validateMcp({ ...base, mcpServers: { bad: { type: 'stdio', command: 'node', cwd: './../escape' } } }), + /cwd must be/u, + ); + assert.throws( + () => validateMcp({ ...base, mcpServers: { bad: { type: 'stdio', command: 'node', cwd: '${PLUGIN_ROOT}/../etc' } } }), + /cwd must be/u, + ); +}); + +test('validateHooksDocument pins the $schema URL to the proposal (R4-3)', () => { + // Wrong URL is now rejected with the new pin. + assert.throws( + () => validateHooksDocument({ + $schema: 'https://example.com/wrong/schema.json', + hooks: { SessionStart: [{ command: 'node' }] }, + }, 'hooks.json'), + /\$schema must equal/u, + ); + // Empty string is now rejected (the old "length > 0" check would + // still pass an empty string; the new pin wouldn't, because the + // empty string doesn't equal the proposal URL). + assert.throws( + () => validateHooksDocument({ + $schema: '', + hooks: { SessionStart: [{ command: 'node' }] }, + }, 'hooks.json'), + /\$schema must equal/u, + ); +}); + test('validateHookEntry type-checks matcher, pattern, regex, glob, once, timeout', () => { assert.throws(() => validateHookEntry({ command: 'node', matcher: 123 }, 'hook'), /matcher must be a non-empty string/u); assert.throws(() => validateHookEntry({ command: 'node', pattern: '' }, 'hook'), /pattern must be a non-empty string/u); @@ -251,6 +320,164 @@ test('record.mjs writes state under PLUGIN_DATA even when it is outside PLUGIN_R } }); +// Round-4 fix (R4-2): the previous ensureContained only did +// `path.resolve` (lexical normalization), which is bypassed when +// PLUGIN_DATA itself is reached through a symlink. For example: +// +// PLUGIN_DATA = /tmp/data (realpath = /var/srv/data) +// realpath('/tmp/data') = '/var/srv/data' +// +// Lexical: startsWith('/tmp/data/') -> true -> "contained" (false positive) +// Real: startsWith('/var/srv/data/') -> true -> contained (the truth) +// +// The hard case is when a SUBDIRECTORY of PLUGIN_DATA is a symlink +// to outside. Lexical containment passes (the symlink lives under +// the lexical root), but real containment fails (the realpath of +// the target is outside the realpath of the root). +test('record.mjs refuses to write through a symlink in PLUGIN_DATA that escapes the root (R4-2)', async () => { + if (process.platform === 'win32') { + // Windows symlinks require admin or developer mode; the existing + // tests in this file already exercise the non-symlink code path, + // and the contract that the symlink case fails is enforced by + // the realpath-based check. Skip on Windows to keep CI green; + // POSIX CI is the real evidence. + return; + } + const tmp = await mkdtemp(path.join(tmpdir(), 'hooks-symlink-')); + const { spawn } = await import('node:child_process'); + const { symlink, mkdir, writeFile: writeFileRaw } = await import('node:fs/promises'); + try { + const data = path.join(tmp, 'plugin-data'); + const outside = path.join(tmp, 'outside'); + await mkdir(data, { recursive: true }); + await mkdir(outside, { recursive: true }); + // Create a sentinel file outside the data root. + await writeFileRaw(path.join(outside, 'pwned.json'), '{"records":[]}', 'utf8'); + // Symlink data/link -> outside so PLUGIN_DATA/link/pwned.json + // lexically lives under PLUGIN_DATA but realpath-wise lives + // under the outside dir. + await symlink(outside, path.join(data, 'link'), 'dir'); + const root = path.join(tmp, 'plugin'); + await mkdir(root, { recursive: true }); + const script = path.join(process.cwd(), 'examples', 'hello-mcode-hooks', 'io.minimax.mcode', 'hooks', 'scripts', 'record.mjs'); + const code = await new Promise((resolveP, rejectP) => { + const child = spawn(process.execPath, [script, '--event', 'SessionStart', '--state', '${PLUGIN_DATA}/link/pwned.json'], { + env: { ...process.env, PLUGIN_ROOT: root, PLUGIN_DATA: data }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + child.stdin.end(JSON.stringify({ toolName: 'Bash' })); + let stderr = ''; + child.stderr.on('data', (chunk) => { stderr += chunk.toString('utf8'); }); + child.on('error', rejectP); + child.on('exit', (c) => resolveP(c)); + }); + // record.mjs swallows the error (Observer must never affect + // agent behavior), so we can't observe the throw directly. + // The observable signal is that the file outside PLUGIN_DATA + // was NOT modified: it still contains the sentinel bytes, not + // a JSON envelope with `records`. + const outsideBytes = await import('node:fs/promises').then((m) => m.readFile(path.join(outside, 'pwned.json'), 'utf8')); + assert.equal(outsideBytes, '{"records":[]}', + `record.mjs must not have written through the symlink (got: ${outsideBytes})`); + // The exit code is 0 because errors are swallowed; the contract + // is that the file system is unchanged. The code is exposed for + // diagnostic purposes only. + assert.equal(code, 0, `record.mjs exit code is 0 (errors swallowed) but state must not have escaped: got ${code}`); + } finally { + await rm(tmp, { recursive: true, force: true }); + } +}); + +// Round-4 fix (R4-4): the previous roundtrip tests only exercised +// SessionStart. The hello-mcode-hooks example ships with +// SessionStart / SessionEnd / PreToolUse entries, and CI only proved +// the first one. These tests run the bundled script with the +// --event flag for the other two. +test('record.mjs handles SessionEnd via the bundled script (R4-4)', async () => { + const tmp = await mkdtemp(path.join(tmpdir(), 'hooks-end-')); + const { spawn } = await import('node:child_process'); + try { + const root = path.join(tmp, 'plugin'); + const data = path.join(tmp, 'data'); + await mkdir(root, { recursive: true }); + await mkdir(data, { recursive: true }); + const stateFile = path.join(data, 'state.json'); + const script = path.join(process.cwd(), 'examples', 'hello-mcode-hooks', 'io.minimax.mcode', 'hooks', 'scripts', 'record.mjs'); + await new Promise((resolveP, rejectP) => { + const child = spawn(process.execPath, [script, '--event', 'SessionEnd', '--state', '${PLUGIN_DATA}/state.json'], { + env: { ...process.env, PLUGIN_ROOT: root, PLUGIN_DATA: data }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + child.stdin.end(JSON.stringify({ sessionId: 'abc-123' })); + child.on('error', rejectP); + child.on('exit', (c) => { if (c === 0) resolveP(); else rejectP(new Error(`exit ${c}`)); }); + }); + const written = JSON.parse(await import('node:fs/promises').then((m) => m.readFile(stateFile, 'utf8'))); + assert.equal(written.records.length, 1); + assert.equal(written.records[0].event, 'SessionEnd'); + assert.deepEqual(written.records[0].payloadKeys, ['sessionId']); + } finally { + await rm(tmp, { recursive: true, force: true }); + } +}); + +test('record.mjs handles PostToolUse via the bundled script (R4-4)', async () => { + const tmp = await mkdtemp(path.join(tmpdir(), 'hooks-post-')); + const { spawn } = await import('node:child_process'); + try { + const root = path.join(tmp, 'plugin'); + const data = path.join(tmp, 'data'); + await mkdir(root, { recursive: true }); + await mkdir(data, { recursive: true }); + const stateFile = path.join(data, 'state.json'); + const script = path.join(process.cwd(), 'examples', 'hello-mcode-hooks', 'io.minimax.mcode', 'hooks', 'scripts', 'record.mjs'); + await new Promise((resolveP, rejectP) => { + const child = spawn(process.execPath, [script, '--event', 'PostToolUse', '--state', '${PLUGIN_DATA}/state.json'], { + env: { ...process.env, PLUGIN_ROOT: root, PLUGIN_DATA: data }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + child.stdin.end(JSON.stringify({ toolName: 'Bash', toolResult: { stdout: 'hello', exitCode: 0 } })); + child.on('error', rejectP); + child.on('exit', (c) => { if (c === 0) resolveP(); else rejectP(new Error(`exit ${c}`)); }); + }); + const written = JSON.parse(await import('node:fs/promises').then((m) => m.readFile(stateFile, 'utf8'))); + assert.equal(written.records.length, 1); + assert.equal(written.records[0].event, 'PostToolUse'); + assert.deepEqual(written.records[0].payloadKeys, ['toolName', 'toolResult']); + } finally { + await rm(tmp, { recursive: true, force: true }); + } +}); + +test('record.mjs handles PreToolUse via the bundled script (R4-4)', async () => { + const tmp = await mkdtemp(path.join(tmpdir(), 'hooks-pre-')); + const { spawn } = await import('node:child_process'); + try { + const root = path.join(tmp, 'plugin'); + const data = path.join(tmp, 'data'); + await mkdir(root, { recursive: true }); + await mkdir(data, { recursive: true }); + const stateFile = path.join(data, 'state.json'); + const script = path.join(process.cwd(), 'examples', 'hello-mcode-hooks', 'io.minimax.mcode', 'hooks', 'scripts', 'record.mjs'); + await new Promise((resolveP, rejectP) => { + const child = spawn(process.execPath, [script, '--event', 'PreToolUse', '--state', '${PLUGIN_DATA}/state.json'], { + env: { ...process.env, PLUGIN_ROOT: root, PLUGIN_DATA: data }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + child.stdin.end(JSON.stringify({ toolName: 'Bash', toolInput: { command: 'rm -rf /' } })); + child.on('error', rejectP); + child.on('exit', (c) => { if (c === 0) resolveP(); else rejectP(new Error(`exit ${c}`)); }); + }); + const written = JSON.parse(await import('node:fs/promises').then((m) => m.readFile(stateFile, 'utf8'))); + assert.equal(written.records.length, 1); + assert.equal(written.records[0].event, 'PreToolUse'); + // payloadKeys is sorted alphabetically (see record.mjs). + assert.deepEqual(written.records[0].payloadKeys, ['toolInput', 'toolName']); + } finally { + await rm(tmp, { recursive: true, force: true }); + } +}); + test('record.mjs enforces MAX_STATE_BYTES and trims older records', async () => { const tmp = await mkdtemp(path.join(tmpdir(), 'hooks-cap-')); const { spawn } = await import('node:child_process'); From 4f22672c676b19f9d64ba2785ca34ed214335709 Mon Sep 17 00:00:00 2001 From: antianqi <75944423+antianqi@users.noreply.github.com> Date: Sat, 29 Aug 2026 13:26:07 +0800 Subject: [PATCH 6/6] fix(hooks): align spec text with validator's $schema contract (round-5) Round-5 review on commit 266068e flagged one normative contract inconsistency: proposals/hooks-detailed-spec.md line 320 said the validator accepts any non-empty $schema string, while the validator (scripts/lib/validation.mjs:275) and the same proposal (line 334-335) require $schema to exactly equal the pinned URL. The two statements defined different contracts; the validator code is the authoritative one. This commit removes the stale "non-empty string" bullet from the "validator enforces" list. The exact-equals clause already lives in the same list further down, so the authoritative contract is now stated once and matches the validator assertion. Validation - node --test test/validation.test.mjs: 22/22 pass (unchanged from 266068e; 0 new tests, 0 modified tests) - node --test (full suite): 127/128 pass. The single remaining fail is the pre-existing test/hosted-plugins.test.mjs:15 Windows-only POSIX-path-regex bug; it fails identically before and after this commit and is unchanged by the spec edit. Design compliance - 1 file changed, 1 deletion(-). Only the contradicting bullet is removed; no rewording of neighbouring bullets, no renumbering. - HOOK_SCHEMA constant value (https://minimax.io/schemas/mcode-hooks/0.1.0/hooks.schema.json) is unchanged and still matches the URL cited in the proposal's example (line 225) and the exact-equals clause (line 334-335). --- proposals/hooks-detailed-spec.md | 1 - 1 file changed, 1 deletion(-) diff --git a/proposals/hooks-detailed-spec.md b/proposals/hooks-detailed-spec.md index ff394c0..452a453 100644 --- a/proposals/hooks-detailed-spec.md +++ b/proposals/hooks-detailed-spec.md @@ -317,7 +317,6 @@ The validator **enforces**: - `hooks.json` parses as JSON and is an object (closed schema; any unknown root field is rejected). -- `hooks.json` declares `$schema` as a non-empty string. - Every key under `hooks` is one of the twelve PascalCase event names listed in § "Empirical event catalog". - Each event value is a non-empty array of hook entries.