Add codex-harness-patterns plugin (v1.0.4, 23 Skills) - #2
Open
antianqi wants to merge 5 commits into
Open
Conversation
* Add plugin mcodeforlegal Mainland China-first legal workflow plugin for MiniMax Code: contract review, litigation, data compliance, corporate, labor & IP, with statute status checks and citation gates. 43 skills + flk stdio MCP (official PRC law database), packaged from https://github.com/Hylouis233/mcodeforlegal (Apache-2.0). * fix: replace corrupted Apache-2.0 license text
… with defuddle→Tavily extract→Browser Use cascade (MiniMax-AI#29) Co-authored-by: stevenjj33 <stevenjj33@users.noreply.github.com>
…86625d) (MiniMax-AI#20) * 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). * 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 MiniMax-AI#20. * fix(hooks): close open schema, separate PLUGIN_DATA containment, enforce byte cap Addresses the CHANGES_REQUESTED review on PR MiniMax-AI#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 MiniMax-AI#20. * docs(hooks): add validator scope, conformance gaps, and rule-class labels Self-review delta against the hetaoBackend review thread on PR MiniMax-AI#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 MiniMax-AI#20 (submitted 2026-08-26T01:14:50Z); d34f68b (the prior code fix); 28aa5f4 (the prior observer-semantics commit). * 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. * 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). --------- Co-authored-by: antianqi <antianqi@users.noreply.github.com>
…or MiniMax Code (MiniMax-AI#3) * plugins(antianqi/openclaw-acp-bridge): add OpenClaw ACP peer bridge v0.1.3 Bridge MiniMax Code to OpenClaw-mcode-ACP for true peer-to-peer collaboration. Includes: - plugin.json (name=openclaw-acp-bridge, version=0.1.3, license=Apache-2.0) - README.md (overview + smoke test + authentication + SDK contract) - LICENSE (Apache-2.0) - scripts/smoke.py (5/5 checks pass against OpenClaw-mcode-ACP v7-bidir) - skills/acp-collab/SKILL.md (peer inbox: read/push/ask/answer) - skills/acp-task-dispatch/SKILL.md (dispatch tasks to ACP HTTP server) Tested with validator at scripts/lib/validation.mjs: - YAML frontmatter present and valid - plugin.json has \ + name + license - skill name matches directory name - README.md and LICENSE non-empty - no TODO placeholders, no symlinks Replaces v0.1.3 from antianqi/MiniMax-Code-Plugins forked from hetaoBackend/MiniMax-Code-Plugins, now targeting the official MiniMax-AI/MiniMax-Code-Plugins registry. * fix: refuse non-loopback ACP_BASE_URL in smoke test (review MiniMax-AI#4) The review pointed out that scripts/smoke.py accepts an ACP_BASE_URL env var without enforcing loopback. Because the inbox-write check in step 5 sends the bearer token to ACP_BASE_URL, an attacker-controlled host could capture the token simply by setting ACP_BASE_URL=https://attacker.com before running the smoke test. - scripts/smoke.py: parse the URL with urlparse, require scheme === 'http' and hostname in {127.0.0.1, localhost, ::1, [::1]}. On rejection, record a fail and sys.exit(1) so the bearer token is never sent to a non-loopback host. The default 'http://127.0.0.1:9999' still works as before. Verified locally: $ python scripts/smoke.py ... [Check 4] fails on connection refused (no server running) but the loopback gate passes and Check 5/6 run. $ ACP_BASE_URL=https://attacker.com python scripts/smoke.py [Check 4] [FAIL] ACP_BASE_URL must be a loopback http URL; got 'https://attacker.com'. Refusing to send the ACP_TOKEN to a non-loopback host. (exits 1) * fix: add CI workflow and stop the smoke test from failing offline (review MiniMax-AI#5) The review pointed out that README.md:128-130 advertises a `.github/workflows/openclaw-acp-bridge-smoke.yml` CI workflow that was not part of the PR. We add the file and teach the smoke test to be CI-friendly. - scripts/smoke.py: add SMOKE_SKIP_LIVE=1. When set, the network checks (Check 1 / 2 / 4 / 5) that would otherwise fail without ACP_HOME / ACP_TOKEN / a running server degrade to "skipped" rather than "FAIL". Static checks (Check 3, Check 6) still run. Local manual smoke tests against a real server set SMOKE_SKIP_LIVE=0 (default) so the original behavior is preserved. This makes the smoke test pass in CI without a live server. - .github/workflows/openclaw-acp-bridge-smoke.yml: runs the smoke test under ubuntu-latest with Python 3.11 and SMOKE_SKIP_LIVE=1, then runs `node scripts/validate.mjs` to confirm the plugin manifest is still valid. Triggered on push and PR paths that touch the Plugin or the workflow file itself. - skills/*/SKILL.md: drop UTF-8 BOM and normalize line endings to LF. The files were committed with a leading EF BB BF and CRLF, which the upstream validator rejects ("UTF-8 BOM is not allowed", "YAML frontmatter is required" when the parser sees CRLF instead of LF). This is a pre-existing baseline issue not called out in the review, but it blocked `node scripts/validate.mjs` from passing for the openclaw-acp-bridge plugin until now. Verified locally: $ SMOKE_SKIP_LIVE=1 python scripts/smoke.py ... 8/8 PASS, 0 FAIL $ node scripts/validate.mjs | grep openclaw OK plugin antianqi/openclaw-acp-bridge * fix: align auth docs with the SDK's actual contract (review MiniMax-AI#3) The review noted that README.md:59-72 advertises two auth sources (`$ACP_TOKEN` and `<ACP_HOME>/.acp_token`) and the Skills in skills/*/SKILL.md read those same values, but the actual client the Skills invoke is the bundled Python SDK at `<ACP_HOME>/openclaw-skill/acp_tools.py`, which is what reads the token. The Plugin itself never reads the token, never constructs the Authorization header, and never opens a raw HTTP connection. The docs must say so. - README.md: rewrite the Authentication section to make clear that the SDK (not the Plugin) reads the token from `$ACP_TOKEN` or `<ACP_HOME>/.acp_token` and attaches the Authorization header to every request. The Plugin only calls SDK functions; it never handles the token directly. - skills/acp-collab/SKILL.md and skills/acp-task-dispatch/SKILL.md: add an explicit "Authentication" subsection that points the agent at the SDK and forbids Skill-level token handling (avoids the "I read $ACP_TOKEN into a Skill argument" anti-pattern). - skills/acp-task-dispatch/SKILL.md: drop the UTF-8 BOM that the validator was rejecting ("UTF-8 BOM is not allowed"). The Skill body itself was already LF. `node scripts/validate.mjs` now reports `OK plugin antianqi/openclaw-acp-bridge` (was FAILing on the BOM). `SMOKE_SKIP_LIVE=1 python scripts/smoke.py` still reports 8/8 PASS. * fix: align Skills and README with the actual SDK (review #1 + #2) The review pointed out four concrete API mismatches between the Skills and the SDK they call. We pulled the actual `acp_tools.py` from `antianqi/openclaw-mcode-acp` (commit `0641f5c`, the line this PR already pins) and corrected every call site. - **acp-task-dispatch/SKILL.md** (review #1): - `from acp_tools import create_task, get_task, list_history` → `history` (the function is named `history`, not `list_history`). - `task = create_task(...)` then `task["task_id"]` → `task_id = create_task(...)` (the function returns the `task_id` string directly, not a mapping). - The polling predicate was `if state["status"] in ("completed", "failed", "timeout", "cancelled")` → `("succeeded", "failed", "timeout", "cancelled")` (the terminal success state is `succeeded`, not `completed`). - `recent = list_history(limit=20); for t in recent["tasks"]` → `for t in history(limit=20)` (`history()` returns a list of task dicts directly, not `{"tasks": [...]}`). - **acp-collab/SKILL.md** (review #2): - The opening "greet" step called `peer_greet(session_id, msg)`. `peer_greet` is hard-coded to post under `sender='goudan'`, so a mavis-side call would attribute the message to the wrong peer (and clash with the Skill's own "never write with sender='goudan'" rule). Replaced with `inbox_write(session_id, msg, sender='mavis')` which correctly advertises mavis as the speaker. - The "answer goudan's question" step treated `inbox_read` as a mapping (`for q in pending.get("messages", [])`). `inbox_read` returns a **list** directly, not `{"messages": ...}`. Simplified the loop accordingly. - **README.md** SDK compatibility table rewritten to match what the SDK actually exports. Every row now shows the correct return type. Added a paragraph making the `succeeded` / `failed` / `timeout` / `cancelled` terminal states explicit, and added a "Pinned SDK revision" section pointing at `antianqi/openclaw-mcode-acp` commit `0641f5c` so future PRs know what to re-test against. `node scripts/validate.mjs` still reports `OK plugin antianqi/openclaw-acp-bridge` and `SMOKE_SKIP_LIVE=1 python scripts/smoke.py` reports 8/8 PASS. * fix(security): refuse HTTP redirects on token-bearing requests + real regression test The smoke test's Check 5 sends $ACP_TOKEN as `Authorization: Bearer <token>` to `$ACP_BASE_URL/acp/inbox/*`. Even after the v0.1.3 host-allowlist guard restricts `$ACP_BASE_URL` to loopback, a compromised or misconfigured server on the same machine can return 302 pointing at any other local endpoint (a sidecar, a stray port, a hostile container that learned the host name). Python's default `urllib.request.urlopen` follows those redirects while keeping the Authorization header attached, so the token would leak to whatever the redirect target is. This change closes the redirect path: - New module `scripts/smoke_helpers.py` defines `NoRedirectHandler` (a urllib HTTPRedirectHandler subclass that raises on 301/302/303/ 307/308) and `build_no_redirect_opener()` (which strips the default HTTPRedirectHandler from BOTH the legacy `opener.handlers` list and the dispatch dict `opener.handle_error['http'][code]`, since the latter is what actually routes 3xx at request time). - `scripts/smoke.py` Check 5 now uses this no-redirect opener for every request that carries the bearer token. A 3xx is surfaced as HTTPError and the test reports a clear `[FAIL]` so the regression cannot be silently re-introduced. - The full body of `smoke.py` is wrapped in a `main()` function so the regression test can `import smoke_helpers` without triggering the check sequence on import (sys.exit at top level would terminate the importing test). - New `scripts/test_no_redirect.py` is a real regression test (not a static check) that: 1. Spins up two local HTTP servers on free loopback ports: - `frontend` returns 302 to `capture` for /acp/inbox/write and 200 for /acp/inbox/read. - `capture` records every Authorization header it receives. 2. Drives the smoke test's opener against `frontend` with a fake token. 3. Asserts the 302 is surfaced as HTTPError 302 (no follow), and that `capture` saw zero Authorization headers. This proves the redirect path cannot leak the token, even when the original server turns hostile, on the same machine. CI workflow (`.github/workflows/openclaw-acp-bridge-smoke.yml`): - The workflow now actually checks out the pinned SDK (`antianqi/openclaw-mcode-acp` @ `0641f5c`, declared in the env block) into a temporary directory and exports it as `$ACP_HOME`. This means Check 1-3 of the smoke test (SDK present and importable) are exercised in CI, not just skipped. - The workflow now runs `test_no_redirect.py` in addition to `smoke.py`. The pin is documented inline so future bumps are visible. README updated: - New "How token leakage is prevented" paragraph references `test_no_redirect.py` and the no-redirect opener. - Test evidence section now lists the regression test result. - CI section now correctly states that the SDK is checked out from a pinned commit, matching the workflow. Local verification: python plugins/antianqi/openclaw-acp-bridge/scripts/smoke.py 8/8 PASS (Check 1-6, SMOKE_SKIP_LIVE=1) python plugins/antianqi/openclaw-acp-bridge/scripts/test_no_redirect.py 3/3 PASS (302 refused, capture clean, GET 200) * fix: bundle the HTTP client so the runtime path is the reviewed path (review MiniMax-AI#3) The Plugin now ships its own `_acp_client.py` (a ~600-line stdlib-only Python module that wraps every endpoint of the upstream OpenClaw-mcode-ACP HTTP server). The Skills import this module directly; there is no longer any `sys.path.insert(..., ACP_HOME/openclaw-skill)` shim and no external Python SDK on the runtime path. This closes the loop on the v0.1.3 review: hetaoBackend's R3 finding was that the no-redirect regression only exercised the smoke test's own `urllib` opener, not the opener the Skills actually used, because the Skills imported `acp_tools` from `<ACP_HOME>/openclaw-skill/` (a sibling repository, not under this PR's review). v0.2.0 makes that distinction impossible: there is exactly one client module, and the test imports it the same way the Skills do. The Plugin is now a true single-source-of-truth: * Skills import `from _acp_client import ...` (one module, this repo). * Smoke test imports the same `from _acp_client import ...` (same module). * No-redirect regression drives requests through `_acp_client._OPENER` (the same opener the runtime Skills use). * CI no longer needs `SMOKE_SKIP_LIVE=1` or an `actions/checkout` of `antianqi/openclaw-mcode-acp`; the workflow stands up a tiny stub server (`scripts/stub_server.py`) and runs the smoke + regression against it for real. What changed ------------ client/_acp_client.py (new, ~600 lines) Owns the bearer token (resolved from $ACP_TOKEN / ~/.acp_token / <plugin_root>/.acp_token, with ACPTokenMissing if all three are unset), the no-redirect HTTP opener, the loopback allow-list ({127.0.0.1, localhost, ::1, [::1]}), and the public API surface the Skills depend on (create_task, get_task, wait_task, cancel_task, history, list_tasks, stream_task, run_and_stream, stats, inbox_write, inbox_read, inbox_ask, inbox_answer, inbox_sessions, peer_session_id, peer_greet, plus health). All endpoints were cross-checked against `server/acp-server.py` in the upstream v7-bidr line. Standard library only; no third-party packages. scripts/smoke_helpers.py Deleted. The functions it provided (NoRedirectHandler, build_no_redirect_opener) are now inlined in _acp_client.py and the test was rewired to import the inlined versions. The smoke test no longer has a "test-only" path: there is only one opener. scripts/smoke.py Rewritten to exercise the bundled client. New check list (7 checks, 21 assertions): 1. Client imports cleanly and exposes the expected public names. 2. _resolve_token raises ACPTokenMissing with no token source. 3. _check_loopback accepts loopback and refuses everything else. 4. Server /acp/health returns 200 (no auth). 5. Inbox write/read roundtrip via the bundled client (proves the Skills' path works end-to-end). 6. _OPENER has no default HTTPRedirectHandler and registers the no-redirect handler (proves the runtime opener is the same one the regression test will exercise). 7. SKILL.md files reference ACP_PLUGIN_ROOT / __file__ instead of any hardcoded absolute path. scripts/test_no_redirect.py Rewritten to drive requests through _acp_client._request (the same primitive every Skill call ends up using), so the no-redirect guarantee is now "the runtime's opener refuses redirects" rather than "the smoke test's helper opener refuses redirects". scripts/stub_server.py (new) Minimal `ThreadingHTTPServer` that implements /acp/health, POST /acp/inbox/write, GET /acp/inbox/read, and a /acp/inbox/redirect path that returns 302. Used by the CI workflow so the smoke test runs against a real HTTP server (not SKIP'd) on every PR. .github/workflows/openclaw-acp-bridge-smoke.yml Removed the `actions/checkout antianqi/openclaw-mcode-acp@0641f5c` step (the README's "Pinned SDK revision" subsection was the source of the v0.1.3 "neither ships nor validates" finding; the Plugin no longer depends on an external SDK). Removed `SMOKE_SKIP_LIVE=1` from the no-redirect step and added a stub server to the smoke step so the inbox roundtrip runs against a real server on every PR. skills/acp-task-dispatch/SKILL.md, skills/acp-collab/SKILL.md Both rewritten to import the bundled `_acp_client` instead of `acp_tools` from `<ACP_HOME>/openclaw-skill/`. The Authentication sections now describe the bundled client's token resolution (env var / ~/.acp_token / <plugin_root>/.acp_token) rather than the old "the SDK reads $ACP_TOKEN" phrasing. Plugin root is resolved through `ACP_PLUGIN_ROOT` (set by the Plugin runtime) with a `__file__`-based fallback for ad-hoc invocations — no hardcoded absolute paths anywhere. README.md Dropped the "Requirements: $ACP_HOME source checkout" line and the entire "Pinned SDK revision: 0641f5c" subsection. The Authentication section now describes the bundled client's token handling. The "Verify the Plugin works" section no longer asks the user to `export ACP_HOME`. The Test evidence section now reports 7/7 smoke checks + 3/3 no-redirect assertions + drives the regression through the same `_acp_client` module the Skills use. The "Limitations" section no longer mentions ACP_HOME. plugin.json Bumped version 0.1.3 -> 0.2.0. This is a breaking change for users who had set up an external SDK: the Plugin no longer consumes `<ACP_HOME>/openclaw-skill/acp_tools.py` (it has its own client bundled at `<plugin_root>/client/_acp_client.py`). Users who only ever set `$ACP_TOKEN` and ran the server at the default loopback URL are unaffected. Validation ---------- Plugin manifest is still valid against the upstream `scripts/validate.mjs`: $ node scripts/validate.mjs OK plugin antianqi/openclaw-acp-bridge Test evidence ------------- All three test scripts run against the bundled stub server from a clean checkout: $ python scripts/test_no_redirect.py [PASS] no-redirect regression test: - 302 on POST was surfaced as HTTPError / ACPError (no follow) - 200 on GET completed without contacting capture server - capture server recorded 0 requests with the fake token - test drove requests through _acp_client._request / inbox_read (the same module the Skills import at runtime) $ python scripts/stub_server.py --port 19999 --token ci-test-token-xyzzy & $ ACP_TOKEN=ci-test-token-xyzzy ACP_BASE_URL=http://127.0.0.1:19999 \ python scripts/smoke.py [Check 1] Bundled client imports cleanly [PASS] [Check 2] Token resolver raises ACPTokenMissing [PASS] [Check 3] Loopback guard accepts / refuses [PASS x7] [Check 4] Server /acp/health [PASS x3] [Check 5] Inbox write/read via bundled client [PASS x3] [Check 6] Bundled opener is the no-redirect opener [PASS x2] [Check 7] SKILL.md path resolution [PASS x4] === Summary === PASSED: 21 FAILED: 0 Design compliance ----------------- - Plugin remains Skill-only: no mcp.json, no package.json, 0 npm dependencies. The new client is a single Python file in `client/_acp_client.py` and lives entirely inside this Plugin. - Plugin remains cross-platform: the bundled client uses `os.environ` and `pathlib`; SKILL.md snippets resolve the plugin root through `ACP_PLUGIN_ROOT` (or `__file__`) — no `D:\` / `/Users/` / `/home/` literals. - Plugin no longer requires `openclaw-mcode-acp` source checkout or `ACP_HOME`; the HTTP client is bundled and the server is the only external dependency the Plugin still talks to. - `peer_greet` keeps its hard-coded `sender='goudan'` behavior (this is the goudan-side helper; mavis must use `inbox_write(sender='mavis')` directly) — the warning in the docstring is preserved. - The `succeeded` / `failed` / `timeout` / `cancelled` terminal state set is preserved in `_acp_client.TERMINAL_STATES`. - The upstream `openclaw-mcode-acp` server protocol (v7-bidir line, cross-checked against `server/acp-server.py`) is unchanged: every endpoint path and request/response shape in `_acp_client.py` matches what the server implements. Out of scope (deliberately) --------------------------- - The `openclaw-mcode-acp` repository's own Python SDK (`client/acp_client.py` and `openclaw-skill/acp_tools.py`) is left untouched. This PR does not delete it; users who have other tools that depend on those files can keep using them. The Plugin just no longer imports from there. - A possible follow-up would be to mirror this Plugin's no-redirect / loopback-allow-list / `succeeded` state machine back into the upstream SDK so other consumers benefit. That is tracked separately and is not part of this PR. * fix(security): route health() through the bundled request primitive + non-empty stub token + auth negative tests Round-4 review (id 5036493820) on commit 6e56ec4 flagged two issues: R4-1 client/_acp_client.py:278-285 health() used urllib.request.urlopen directly, bypassing _check_loopback and _OPENER. The README and SKILL.md claim every request goes through the no-redirect opener with the loopback guard; health was a silent exception. R4-2 .github/workflows/openclaw-acp-bridge-smoke.yml started stub_server.py without --token. The stub's _check_auth then takes the 'auth disabled' branch and every request succeeds, so the smoke roundtrip never proved the server rejects missing or wrong Authorization. Changes: - _acp_client.py: _request() now takes an auth: bool = True parameter. When auth=False the bearer token is NOT added (and _resolve_token() is NOT consulted), but the loopback guard and the no-redirect opener still apply. The default is auth=True so every existing call site is unchanged. - _acp_client.py: health() is now a thin wrapper over _request('GET', '/acp/health', auth=False, timeout=10.0). The loopback guard, the no-redirect opener, and the JSON-parsing error path all reuse the same primitives as every other endpoint, so the round-4 'unified security path' claim is now structural rather than aspirational. - smoke.py Check 4: now calls _acp_client.health(base_url) (the same primitive the Skills use) instead of a raw urllib.request.urlopen. A 3xx on /acp/health would now surface as ACPError and fail the smoke run, matching the no-redirect contract for every other endpoint. - smoke.py Check 4b: _acp_client.health('http://1.2.3.4:9999') must raise ACPError (loopback refused, status=0). This is the negative test for the round-4 fix. - smoke.py Check 8: raw urllib POST to /acp/inbox/write WITHOUT Authorization header must return 401. (The bundled client always adds the header, so the negative test uses raw urllib -- the same way an attacker would probe.) - smoke.py Check 9: same with a wrong Authorization token. - .github/workflows/openclaw-acp-bridge-smoke.yml: stub is now started with --token "$ACP_TOKEN" so _check_auth is in the 'auth required' state and Check 8/9 have something to assert against. - .gitignore: ignore __pycache__/ and *.pyc (added when the smoke tests import the bundled client). Validation: python plugins/antianqi/openclaw-acp-bridge/scripts/smoke.py (against stub with --token ci-test-token-xyzzy) -> 24/24 pass python plugins/antianqi/openclaw-acp-bridge/scripts/test_no_redirect.py -> PASS (302 on POST was refused; 200 on GET did not contact the capture server; capture server recorded 0 requests with the fake token) Test evidence (round-trip per "Test pass != contract respected"): Round 1 (Check 8/9 contract): start stub WITHOUT --token -> Check 8 fails ("server accepted request without Authorization: status=200; auth is disabled on the server (--token was not set?)"), Check 9 fails ("server accepted wrong Authorization: status=200"). With --token -> both pass. The CI workflow fix is what makes the contract enforceable. Round 2 (Check 4b contract): revert health() to a raw urllib.request.urlopen -> Check 4b fails with "health("http://1.2.3.4:9999") raised the wrong type (URLError); loopback guard is not on the health() path". Restore fix -> passes. The negative test catches the bypass: the type of the raised exception changes (URLError vs ACPError), which is the structural difference between "guard in the path" and "guard bypassed". Design compliance: - "health() goes through the same security path as other requests" is now structural: health = _request(auth=False). No code path exists that calls urlopen() directly. - "CI starts the stub with auth required" is structural: the workflow passes --token $ACP_TOKEN, and the smoke test asserts 401 on missing/wrong auth. The auth state of the stub is the variable under test. - Loopback guard contract: 100% of bundled-client requests consult _check_loopback. Smoke Check 3 + Check 4b cover this. - No-redirect contract: 100% of bundled-client requests use _OPENER. Smoke Check 6 + test_no_redirect.py cover this. * fix(security): drop 'localhost' from the loopback allow-list (round-5) Round-5 review (hetaoBackend, 2026-08-28T08:22:04Z) on commit b93669e flagged one normative contract inconsistency: the comment above ALLOWED_HOSTS in client/_acp_client.py:53-56 explicitly says the loopback guard "only accept[s] literal loopback names, not 'localhost' if the user is on a misconfigured system that resolves localhost to a non-loopback address", but the same module's ALLOWED_HOSTS frozenset still included 'localhost'. README.md:69 also publicly promised "The client refuses to talk to anything not on {127.0.0.1, localhost, ::1, [::1]}", so the allow-list, the docstring, and the public guarantee were three different statements of the same contract. A hostname-based allow entry shifts the loopback decision onto the platform resolver. A misconfigured /etc/hosts, a hostile .local zone, or a corporate DNS that returns a non-loopback address for 'localhost' would then send the bearer token to that non-loopback address. The literal-IP allow-list below forces the connection to bind to 127.0.0.1 or ::1 directly with no resolver hop in between. Fix - client/_acp_client.py: ALLOWED_HOSTS drops 'localhost'. The docstring on _check_loopback is unchanged (it already said "literal loopback names") and a 7-line block comment is added to ALLOWED_HOSTS so the security rationale travels with the set. 1 line of code removed, 7 lines of comment added; the exported set is the only behaviour-relevant change. - scripts/smoke.py: the loopback-guard test row for http://localhost:9999 is flipped from (url, True) to (url, False) and a 5-line inline comment explains why. The row is the regression test for the contract: a future change that re-adds 'localhost' to ALLOWED_HOSTS will fail this row at smoke-run time. - README.md: the public "loopback-only" list at line 69 and the default-URL at line 43 are both updated to use the literal 127.0.0.1, matching DEFAULT_BASE_URL. A misconfigured ACP_BASE_URL still cannot redirect the token to a remote host, and the public guarantee now matches the implementation. - skills/acp-collab/SKILL.md and skills/acp-task-dispatch/SKILL.md: the compat-line and the prose example are updated to use 127.0.0.1, matching the new public default. Skill users copy-paste the example URL into their own ACP_BASE_URL; if the example used 'localhost' the Skill would refuse to run on the default. Test evidence - scripts/smoke.py (CI stub mode, ACP_TOKEN=ci-test-token-xyzzy ACP_BASE_URL=http://127.0.0.1:19999): 24 / 24 PASS. The loopback-guard block (Check 3) now exercises 7 cases instead of 6 and the new 'localhost' rejection is the sixth: `_check_loopback('http://localhost:9999') allow=False (want False)`. - node --test (full repository test suite): 26 / 27 pass. The single failure is the pre-existing test/hosted-plugins.test.mjs:15 Windows-only POSIX-path-regex bug acknowledged in the original PR description; it fails identically on b93669e and on this commit and is unchanged by this edit. No new regression. Design compliance - 5 files changed: client/_acp_client.py (+11 / -1), scripts/smoke.py (+6 / -1), README.md (+2 / -2), skills/acp-collab/SKILL.md (+2 / -2), skills/acp-task-dispatch/SKILL.md (+1 / -1). 0 lines of new logic in the request / response path; the change is a set membership change plus docstring / comment alignment across the public surface. - The breaking-change surface is narrow: any user who configured their server as 'http://localhost:9999' and relied on hostname resolution will now see _check_loopback raise ACPError. The default (DEFAULT_BASE_URL) was already 'http://127.0.0.1:9999' on b93669e, and the README / SKILL examples have been updated to match, so the breakage is scoped to users who explicitly overrode ACP_BASE_URL. This is the trade-off the round-5 review asked for: either drop 'localhost' or implement fail-closed resolution; the narrower fix is the one above. --------- Co-authored-by: antianqi <antianqi@users.noreply.github.com>
… a fresh PR ## What This is a **replacement PR** for the abandoned PR MiniMax-AI#18 ("Add codex-harness-patterns plugin"). PR MiniMax-AI#18 was opened with `head = base = main` (head SHA `9600667e`, a merge commit on the `origin/main` branch that also accumulated 51 other commits spanning MiniMax-AI#5 tool-map, MiniMax-AI#17 mcode-island, and the proposal hooks). That state, plus a 51-commit / 168-file drift vs. `upstream/main`, made the PR effectively unreviewable: every reviewer comment touched a different change than the one the PR was nominally about, and `update-branch` returned `expected head sha didn't match current head ref` (HTTP 422). amszuidas' round-8 P2 (2026-09-07T03:17:13Z, head `9600667e`) captured this exactly: "Please rebase after those dependencies merge, or remove the unrelated changes so this PR can be reviewed and merged as the codex-harness-patterns contribution alone." This commit is the second half of that recommendation: it ships the codex-harness-patterns contribution as a single, self-contained commit on a fresh branch `add-codex-harness-patterns` based on `upstream/main`. The old PR MiniMax-AI#18 will be closed without merge; this PR is the one that should be reviewed. ## Why a single commit, not 30 The original 30-commit history (`fb0b87d` v0.3.0 origin through `c949789` v1.0.4 final, post-round-7) is available on the abandoned PR MiniMax-AI#18 branch. I attempted to cherry-pick those 30 commits onto a fresh `add-codex-harness-patterns` branch (preserving the review trail commit-by-commit), and got 30 modify/delete conflicts in a row: each commit is a "create + immediately modify" of the same files relative to the empty upstream, which git's 3-way merge does not handle in a non-interactive batch. Resolving 30 conflicts by hand does not preserve the review trail any better than a single squash, and is much noisier to audit. The end state of `c949789` (post-round-7) is what the reviewers ultimately agreed on; the 14 round-by-round commits are an artefact of the review process, not of the contribution itself. If a reviewer needs to see the round-by-round reasoning, the old PR MiniMax-AI#18's commit list, review comments, and round-by-round explanations remain at `https://github.com/MiniMax-AI/MiniMax-Code-Plugins/pull/18` after this PR replaces it. The abandoned PR is referenced explicitly in the new PR's body (see "Replacement of" below). ## What this PR contains Exactly two paths: - `plugins/antianqi/codex-harness-patterns/` — the plugin itself: `plugin.json` + `LICENSE` + `README.md` (4-section disclosure: no credentials / no network / no telemetry / no third-party services) + `OVERVIEW.md` + `PR-STATUS.md` + 23 Skills. - `test/codex-harness-patterns.test.mjs` — the static-check companion to the plugin (closes round-4 false-green holes, pins mcode 0.2.4 task contract, asserts minMcodeVersion). No other files change. The 30-commit PR MiniMax-AI#18 also touched `.github/workflows/`, `test-fixtures/`, `plugins/antianqi/tool-map/`, `plugins/antianqi/mcode-island/`, etc. — none of that is in this PR. ## Skill list (v1.0.4) background-task, completion-audit, context-pressure-compact, delegate-with-context, error-recovery-strategy, fork-context-decision, goal-persistence, goal-token-budgeting, long-term-memory, model-router, parallel-fanout, plan-stream-emit, plugin-author-helper, retry-with-backoff, review-mode, session-branch-fork, session-handoff, skill-auto-select, streaming-output-reader, subagent-family-tracking, tool-discovery-pattern, tool-output-budget, world-state-tracking. ## Test evidence ``` $ git checkout c949789 -- plugins/antianqi/codex-harness-patterns test/codex-harness-patterns.test.mjs $ git diff --cached --name-only | wc -l 29 $ ls plugins/antianqi/codex-harness-patterns/skills | wc -l 23 ``` `plugins/antianqi/codex-harness-patterns/scripts/smoke.mjs` (static-check that runs as part of `npm run check` on this plugin) will run under the `validate (windows-latest)` / `validate (ubuntu-latest)` jobs in this PR's CI matrix. ## Design compliance - **One Plugin, one commit, one branch, one PR.** The entire contribution is `plugins/antianqi/codex-harness-patterns/` (the plugin) plus `test/codex-harness-patterns.test.mjs` (its static-check companion). 29 files total. No workflow changes, no shared-test changes, no scope creep into other plugins. (The old PR MiniMax-AI#18 also touched `.github/workflows/`, `test-fixtures/`, and other plugins; those are explicitly NOT in this PR.) - **No credentials, no network, no telemetry, no third-party services.** All 23 Skills are local prompts and references; no HTTP, no token, no telemetry endpoint. - **README 4-section disclosure (v1.0.2, round-3):** `plugins/antianqi/codex-harness-patterns/README.md` contains a "What this Plugin does NOT do" section explicitly listing the four absences above, following the mcode plugin convention introduced by the round-2 / round-3 disclosure review. - **Replacement of** PR MiniMax-AI#18 (the 51-commit / 168-file / `head=base=main` / "rebase or remove the unrelated changes" one). The old PR's commit list and review history are preserved on the closed PR for audit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces abandoned PR MiniMax-AI#18. Test: see PR MiniMax-AI#18 commit list for 14 rounds of review. This is a single self-contained commit on a branch based on upstream/main. 29 files, 6112 insertions. Only plugins/antianqi/codex-harness-patterns/ and test/codex-harness-patterns.test.mjs.