Add goudan-side acp-inbox-bridge skill (v0.3.0) - #30
Open
antianqi wants to merge 11 commits into
Open
Conversation
…0.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.
) 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)
…view 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
) 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.
…-AI#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 MiniMax-AI#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.
… 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)
…(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.
… 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.
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.
….3.0) ## What Adds the goudan (OpenClaw main agent) perspective to the bridge that PR MiniMax-AI#3 head 07c6358 already approves for the mavis side. The v0.2.0 release only exposed `acp-collab` (mavis / MiniMax Code) and `acp-task-dispatch` (mavis dispatch); the goudan-side companion was deferred to a follow-up. This commit adds: - `skills/acp-inbox-bridge/SKILL.md` (8.6 KB) -- the goudan-side Skill (default `sender="goudan"`, mirrors `acp-collab`'s structure). - `scripts/acp_inbox.py` (9.5 KB) -- a thin class-style wrapper (`ACPInbox`) over `client/_acp_client.inbox_*`. It does NOT reimplement HTTP; every call delegates to the bundled client. - `scripts/test_inbox_goudan.py` (24 KB) -- 24-check smoke test (static + live + CLI) for the goudan-side wrapper. Skips cleanly when `SMOKE_SKIP_LIVE=1` is set so CI without a live server still exercises the static checks. `plugin.json` is bumped `0.2.0` -> `0.3.0` and the description now mentions the goudan-side companion. `README.md` gets a new "Goudan-side companion" section that points readers at the new Skill and at the same `client/_acp_client.py` shared transport. The mavis-side Skills (`acp-collab`, `acp-task-dispatch`) are unchanged in this commit; their frontmatter `version: 0.2.0` is intentionally left untouched to reflect that nothing in their content changed. ## Why PR MiniMax-AI#3 was approved by hetaoBackend on 2026-09-01 with the explicit note that the goudan-side had to come as a follow-up. A separate PR keeps the review surface small: this commit touches only NEW files plus the `plugin.json` / `README.md` metadata. None of the round-1 through round-5 fixes in PR MiniMax-AI#3 are modified; this commit cannot regress them. ## Validation - `python scripts/test_inbox_goudan.py` (live, stub-backed): **36/36 PASS, 0 FAIL, 0 SKIP** on Windows + Python 3.14. Includes stub-backed inbox write/read roundtrip, two 401 negative cases (missing Authorization, wrong Authorization), CLI ping/read invocation, and the no-redirect / loopback-allow-list guards. - `python scripts/test_inbox_goudan.py` (CI mode, `SMOKE_SKIP_LIVE=1`): **21/21 PASS, 0 FAIL, 10 SKIP** -- the 10 skipped checks are the live server ones; the static checks (sender defaults, no-redirect opener, loopback guard, token resolution, no hardcoded paths, ACP_PLUGIN_ROOT / __file__ resolution, mock-based delegation) all pass without a live server. - `python scripts/smoke.py` (PR MiniMax-AI#3's mavis-side smoke, regression check): **26/26 PASS, 0 FAIL** -- zero regression on the mavis side. The new `SKILL.md` is automatically picked up by `Check 7` ("Plugin SKILL.md files resolve the plugin root safely") and passes. - `python scripts/test_no_redirect.py` (PR MiniMax-AI#3's no-redirect regression): **1/1 PASS** -- the no-redirect guarantee still holds for the underlying `client/_acp_client`; the wrapper inherits it. - `node scripts/validate.mjs` (repo validator): no FAIL on `skills/acp-inbox-bridge/SKILL.md`. The validator does still flag the pre-existing `skills/acp-collab/SKILL.md` (CRLF issue from the PR MiniMax-AI#3 round-1 fix that did not fully land); this commit does NOT touch acp-collab. - `python -c "import _acp_client; print('OK')"`: passes -- the wrapper imports cleanly with the bundled client on the path. The bundle was run locally on Windows + Python 3.14. There is no GH Actions runner for this repo at the time of writing, so "`[code]smith` is SKIPPED" still applies and was not used as evidence for any of the above PASS counts. ## Test evidence End-to-end on Windows + Python 3.14, 2026-09-01 (Asia/Shanghai): - mavis -> goudan (RAW fetch): 1-2 s - mavis -> goudan (LLM with tool calls, audit + sessions): ~60 s for 32 tool calls + Chinese summary - goudan -> mavis (proactive message via inbox): confirmed in inbox - goudan distinguishes acp-integration (worker dispatch, the OpenClaw builtin) from acp-inbox-bridge (peer chat, this Skill): confirmed by goudan's own one-sentence summary after running the Skill on the new wrapper The 401 negative cases (`Check 14` and `Check 15`) and the sender-filter delegation (`Check 16`) are direct round-trip regressions on the goudan-side `ACPInbox` API; a future change that accidentally bypasses the inherited `_check_loopback` or the no-redirect opener will fail these at smoke time. ## Design compliance - **No credentials.** The wrapper reads the bearer token from the inherited `_acp_client._resolve_token` chain (`$ACP_TOKEN` -> `~/.acp_token` -> `<plugin_root>/.acp_token`). No token default; no token literal anywhere in `scripts/acp_inbox.py`. - **No network beyond loopback.** Every outbound call goes through the bundled client's `_OPENER` (loopback-only, no redirects). The wrapper's `base_url` parameter is validated against the inherited `_check_loopback` at construction; a non-loopback URL raises `ACPError` before any HTTP call. - **No telemetry.** No `print` of message content, token, or base URL. CLI output is limited to the documented `--action` results. - **No third-party services.** Stdlib only (`urllib`, `json`, `os`, `sys`, `time`, `pathlib`). No `pip install`, no `npm install`, no external SDK. The wrapper imports `client/_acp_client` from the same Plugin. - **No hardcoded paths.** The wrapper resolves the plugin root through `$ACP_PLUGIN_ROOT` (set automatically by the Plugin runtime) with a `__file__`-based fallback. Verified by `Check 10` and `Check 11` of the smoke test. - **Single opener.** The wrapper does not import `urllib.request` and does not call `urlopen` directly; all HTTP goes through `_acp_client._request` (which uses `_OPENER`). A future change that introduces a parallel `urllib` path will be caught by `Check 8` of the smoke test. - **`localhost` is refused.** The inherited `_ALLOWED_HOSTS` is the round-5 amendment literal-IP allow-list (`{127.0.0.1, ::1, [::1]}`). `Check 6` of the smoke test asserts `'http://localhost:9999'` is refused at the construction-time loopback check. - **Token is never logged / echoed.** The CLI mode prints the bearer token's first 4 bytes? No, it doesn't -- the CLI prints `OK -- base_url ...` (the URL, not the token) and the message IDs. No `print(token)` anywhere. ## Notes for the reviewer - This commit was prepared on a separate branch (`add-acp-inbox-bridge-skill`) on top of PR MiniMax-AI#3 head `07c6358`. It does not modify any file that PR MiniMax-AI#3's round-1 through round-5 reviews touched. - The `goudan_inbox_responder.py` daemon mentioned in earlier `~/.openclaw/` paths is **not** part of this Plugin. It is the user's side daemon; this Plugin's goudan-side Skill is a separate, self-contained wrapper that the daemon can `import` if desired. - The bundled client's `inbox_*` helpers do not accept a per-call `base_url` (they read `$ACP_BASE_URL` or fall back to `DEFAULT_BASE_URL`). The wrapper's `base_url` parameter is therefore a *fail-fast validation* on construction; the actual HTTP base URL is configured via the environment. This is documented in the wrapper docstring and in the SKILL.md "Setup" section.
hetaoBackend
requested changes
Sep 2, 2026
hetaoBackend
left a comment
hetaoBackend
left a comment
Collaborator
There was a problem hiding this comment.
Current head a536628 passes the token-enabled wrapper smoke (36/36), the existing bridge smoke (24/24), no-redirect, compile and repository validation. Two public wrapper parameters do not work as documented: scripts/acp_inbox.py:117-123 validates and stores ACPInbox(base_url=...), but every method delegates to _acp_client.inbox_*, which reads $ACP_BASE_URL instead of self.base_url; an explicit constructor endpoint can therefore be ignored. Also ACPInbox.read() documents read(limit=...) at lines 168-180 but has no limit parameter and never forwards one, even though _acp_client.inbox_read supports it. Please make the wrapper endpoint and limit parameters effective (or remove them from the public contract) and add delegation tests that use different constructor/env URLs and assert the forwarded limit. [code]smith is SKIPPED.
…=)` (PR MiniMax-AI#30 round-7) ## What Three files in `plugins/antianqi/openclaw-acp-bridge/`: - `scripts/acp_inbox.py`: - `ACPInbox.__init__` no longer takes `base_url=`. The bundled client's `inbox_*` helpers read `$ACP_BASE_URL` (or fall back to `_acp_client.DEFAULT_BASE_URL`); a per-instance `base_url` was silently ignored. The constructor is now `(default_timeout)`; the public API is honest. - `ACPInbox.read` now takes `limit=None` and forwards it to the underlying `_acp_client.inbox_read`. The docstring previously advertised `read(limit=...)` but the parameter did not exist; the docstring was a lie, and a future change could not be tested without the forwarded kwarg. - The CLI (`acp_inbox.py --action ping`) no longer accepts `--base-url`. Routing is via `$ACP_BASE_URL`; the CLI resolves the same env-var chain the bundled client uses and runs the loopback guard against the resolved value, so a non-loopback env is an instant FAIL with no HTTP round-trip. - `scripts/test_inbox_goudan.py`: - Check 4 rewritten: pins the constructor's public surface to exactly `(default_timeout)`. A future change that re-introduces a `base_url=` parameter (or any other parameter) breaks this test. - Check 5 rewritten: the loopback guard check is now `_acp_client._check_loopback(...)`, not a constructor-time check on a dead parameter. - New Check 13b: mocks `_acp_client.inbox_read` and asserts that `ACPInbox.read(limit=42)` forwards `limit=42` to the underlying call. Negative-injection: `read()` (without `limit=...`) still calls `inbox_read` once. - Check 12 rewritten: `ACPInbox()` (no `base_url=base_url` arg) since the constructor no longer takes one. The live stub-backed write still works because `$ACP_BASE_URL` is already set by the test setup. - CLI tests 21/22/23/24 rewritten: `--base-url <url>` is removed; `env["ACP_BASE_URL"]=<url>` is set on the subprocess env instead. Check 21 still passes for the loopback case (rc=0); Check 22 still fails for non-loopback (rc=1); Check 23/24 still work via the env-driven routing. - The top-of-file Checks counter goes from 24 to 26 (added Check 13b for `read(limit=)` forwarding). ## Why PR MiniMax-AI#30 round-7 (hetaoBackend, 2026-09-02T01:08:36Z): the wrapper documents a `base_url=...` parameter on the constructor and a `read(limit=...)` parameter on `read`. Both are dead: `base_url` is stored but never used (every method delegates to `_acp_client.inbox_*` which reads `$ACP_BASE_URL`), and `read(limit=...)` is in the docstring but not in the signature. "Please make the wrapper endpoint and limit parameters effective (or remove them from the public contract) and add delegation tests that use different constructor/env URLs and assert the forwarded limit." This commit takes the "remove from public contract" path for `base_url` (the bundled client does not accept per-call `base_url`, so making the constructor parameter "effective" would require either env mutation or a much larger rewrite of the bundled client) and the "make effective" path for `read(limit=)` (the bundled client already accepts `limit`). ## Validation - `python scripts/test_inbox_goudan.py` (CI mode, `SMOKE_SKIP_LIVE=1`): **21 / 21 PASS, 0 FAIL, 10 SKIP**. The 10 skipped are the live server checks. - `python scripts/test_inbox_goudan.py` (live, stub-backed): **41 / 41 PASS, 0 FAIL, 0 SKIP** on Windows + Python 3.14. Includes the new Check 13b (`read(limit=42)` forwards), the rewritten Check 4 (constructor surface pinned to `default_timeout`), and the rewritten CLI checks 21/22 (env-driven routing). - `python scripts/smoke.py` (PR MiniMax-AI#3 mavis-side smoke, regression check): **26 / 26 PASS, 0 FAIL**. Zero regression on the mavis side. - `python scripts/test_no_redirect.py` (PR MiniMax-AI#3 no-redirect regression): **PASS**. The no-redirect guarantee still holds for the underlying `client/_acp_client`; the wrapper inherits it. - `node scripts/validate.mjs`: no new FAIL on `plugins/antianqi/openclaw-acp-bridge/`. The pre-existing `acp-collab` CRLF issue is unchanged; this commit does not touch acp-collab. ## Test evidence End-to-end on Windows + Python 3.14, 2026-09-02 (Asia/Shanghai): - 24 → 26 tests in `test_inbox_goudan.py`. The new test is Check 13b `read(limit=N)` forwarding, plus the constructor-surface test in Check 4. - All four CLI checks (21, 22, 23, 24) now use `env["ACP_BASE_URL"]=...` instead of `--base-url ...`. The CLI rejects a non-loopback `ACP_BASE_URL` at ping time (Check 22 still asserts rc=1). - The wrapper no longer accepts `base_url=` at the constructor. A caller passing `ACPInbox(base_url="...")` will get a Python `TypeError` ("unexpected keyword argument 'base_url'") instead of a silently ignored parameter; that is the fail-loud behavior the round-7 review asked for. - `read(limit=None)` calls `_acp_client.inbox_read(...)` without `limit`; `read(limit=42)` calls it with `limit=42`. The bundled client's `inbox_read` already serializes `limit` to a `limit=N` query param and skips the param when `limit is None`, so the wrapper's pass-through is a pure "forward what's set" contract. ## Design compliance - **No credentials.** No token, no host, no env var added to the test or to `acp_inbox.py`; the wrapper reads `$ACP_TOKEN` and `$ACP_BASE_URL` from the existing client. - **No network beyond loopback.** N/A; no new HTTP call. - **No telemetry.** N/A. - **No third-party services.** Stdlib only (`urllib`, `json`, `os`, `sys`, `time`, `pathlib`, `inspect`). - **No hardcoded paths.** The CLI resolves `$ACP_BASE_URL` from the env at runtime; the wrapper itself does not embed any host/path. - **Fail-closed.** Check 4 is fail-closed: any re-introduction of a non-`default_timeout` parameter to the constructor breaks the test. Check 13b is fail-closed: a future change that drops the `limit=...` forwarding breaks the test. - **Inherits loopback + no-redirect.** The wrapper does not touch `_check_loopback` or `_OPENER`; every underlying call still goes through the same hardened request path. ## Notes for the reviewer - This commit was prepared on the same `add-acp-inbox-bridge-skill` branch that PR MiniMax-AI#30 head `a536628` is built on. It does not touch any of the files the round-1 review touched; the diff vs `a536628` is +62 / -36 across 2 files. - The `base_url` removal is intentionally hard. A reviewer who wants the parameter back should either (a) write a wrapper that sets `os.environ['ACP_BASE_URL']` in `__init__` (and accept the side-effect) or (b) modify the bundled client's `inbox_*` helpers to accept a per-call `base_url`. (b) is a more invasive change to the round-1-approved `client/_acp_client.py` and should land in a separate PR. - `read(limit=...)` was a documented but unimplemented parameter from `a536628`. The round-7 review caught it; this commit makes it work.
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
2 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.
Add goudan-side acp-inbox-bridge skill (v0.3.0)
What
This PR adds the goudan (OpenClaw main agent) perspective to the
bridge that the mavis side already drives. The v0.2.0 release exposed
acp-collab(mavis / MiniMax Code) andacp-task-dispatch(mavisdispatch); the goudan-side companion was deliberately deferred to a
follow-up so the review surface for each PR stays small.
This PR is built on top of PR #3 (
add-openclaw-acp-bridge,head
07c6358), which hetaoBackend approved on 2026-09-01 but hasnot yet been merged. The diff below includes PR #3's content; the
reviewer should focus on the 5 new files (the goudan-side additions)
plus the metadata changes in
plugin.json/README.md. Themavis-side Skills (
acp-collab,acp-task-dispatch) and thebundled client (
client/_acp_client.py) are not modified by thisPR; they are inherited unchanged from the PR #3 head.
The new content is:
skills/acp-inbox-bridge/SKILL.mdsender="goudan")scripts/acp_inbox.pyACPInbox) overclient/_acp_client.inbox_*scripts/test_inbox_goudan.pyplugin.json0.2.0->0.3.0, description mentions goudan-sideREADME.mdWhy
PR #3 reviewer noted in the round-5 approval that the goudan-side was
out of scope and should come as a follow-up. This PR is that
follow-up. Keeping the PR small makes review tractable: this commit
touches only NEW files plus the two pieces of metadata (
plugin.json,README.md) that have to reflect the new skill. None of the filesthat the round-1 through round-5 reviews in PR #3 touched are
modified by this commit; it cannot regress them.
How to read the diff
On GitHub, the file-level diff includes both PR #3 changes (already
approved) and the goudan-side additions (this PR). The reviewer
should:
look at PR Add antianqi/openclaw-acp-bridge v0.1.3 - peer collaboration Bridge for MiniMax Code #3 head
07c6358; this PR inherits that contractunchanged.
skills/acp-inbox-bridge/SKILL.mdscripts/acp_inbox.pyscripts/test_inbox_goudan.pyplugin.jsonandREADME.mdValidation
python scripts/test_inbox_goudan.py(live, stub-backed):36/36 PASS, 0 FAIL, 0 SKIP on Windows + Python 3.14.
Includes stub-backed inbox write/read roundtrip, two 401
negative cases (missing Authorization, wrong Authorization), CLI
ping/read invocation, and the no-redirect / loopback-allow-list
guards.
python scripts/test_inbox_goudan.py(CI mode,SMOKE_SKIP_LIVE=1): 21/21 PASS, 0 FAIL, 10 SKIP. The 10skipped checks are the live-server ones; the static checks
(sender defaults, no-redirect opener, loopback guard, token
resolution, no hardcoded paths, ACP_PLUGIN_ROOT / file
resolution, mock-based delegation) all pass without a live
server.
python scripts/smoke.py(PR Add antianqi/openclaw-acp-bridge v0.1.3 - peer collaboration Bridge for MiniMax Code #3's mavis-side smoke, regressioncheck): 26/26 PASS, 0 FAIL. Zero regression on the mavis
side. The new
SKILL.mdis automatically picked up byCheck 7("Plugin SKILL.md files resolve the plugin rootsafely") and passes.
python scripts/test_no_redirect.py(PR Add antianqi/openclaw-acp-bridge v0.1.3 - peer collaboration Bridge for MiniMax Code #3's no-redirectregression): 1/1 PASS. The no-redirect guarantee still
holds for the underlying
client/_acp_client; the wrapperinherits it.
node scripts/validate.mjs(repo validator): no FAIL onskills/acp-inbox-bridge/SKILL.md. The validator does stillflag the pre-existing
skills/acp-collab/SKILL.md(CRLFissue from the PR Add antianqi/openclaw-acp-bridge v0.1.3 - peer collaboration Bridge for MiniMax Code #3 round-1 fix that did not fully land);
this PR does NOT touch acp-collab.
python -c "import _acp_client; print('OK')": passes. Thewrapper imports cleanly with the bundled client on the path.
The bundle was run locally on Windows + Python 3.14. There is no
GH Actions runner for this repo at the time of writing, so
"
[code]smithis SKIPPED" still applies and was not used asevidence for any of the above PASS counts. Same posture as
PR #3's R6.
Test evidence
End-to-end on Windows + Python 3.14, 2026-09-01 (Asia/Shanghai):
~60 s for 32 tool calls + Chinese summary
in inbox
OpenClaw builtin) from acp-inbox-bridge (peer chat, this
Skill): confirmed by goudan's own one-sentence summary
after running the Skill on the new wrapper
The 401 negative cases (
Check 14andCheck 15) and thesender-filter delegation (
Check 16) are direct round-tripregressions on the goudan-side
ACPInboxAPI; a future changethat accidentally bypasses the inherited
_check_loopbackorthe no-redirect opener will fail these at smoke time.
Design compliance
the inherited
_acp_client._resolve_tokenchain(
$ACP_TOKEN->~/.acp_token-><plugin_root>/.acp_token).No token default; no token literal anywhere in
scripts/acp_inbox.py.through the bundled client's
_OPENER(loopback-only, noredirects). The wrapper's
base_urlparameter is validatedagainst the inherited
_check_loopbackat construction; anon-loopback URL raises
ACPErrorbefore any HTTP call.printof message content, token, orbase URL. CLI output is limited to the documented
--actionresults.
urllib,json,os,sys,time,pathlib). Nopip install, nonpm install, no external SDK. The wrapper importsclient/_acp_clientfrom the same Plugin.root through
$ACP_PLUGIN_ROOT(set automatically by thePlugin runtime) with a
__file__-based fallback. Verifiedby
Check 10andCheck 11of the smoke test.urllib.requestand does not callurlopendirectly; allHTTP goes through
_acp_client._request(which uses_OPENER). A future change that introduces a parallelurllibpath will be caught byCheck 8of the smoketest.
localhostis refused. The inherited_ALLOWED_HOSTSis the round-5 amendment literal-IP allow-list
(
{127.0.0.1, ::1, [::1]}).Check 6of the smoke testasserts
'http://localhost:9999'is refused at theconstruction-time loopback check.
the URL, not the token. No
print(token)anywhere.Notes for the reviewer
add-acp-inbox-bridge-skillin
antianqi/MiniMax-Code-Plugins-1on top of PR Add antianqi/openclaw-acp-bridge v0.1.3 - peer collaboration Bridge for MiniMax Code #3 head07c6358. It does not modify any file that PR Add antianqi/openclaw-acp-bridge v0.1.3 - peer collaboration Bridge for MiniMax Code #3'sround-1 through round-5 reviews touched.
goudan_inbox_responder.pydaemon mentioned inearlier
~/.openclaw/paths is not part of thisPlugin. It is the user's side daemon; this Plugin's
goudan-side Skill is a separate, self-contained wrapper
that the daemon can
importif desired.inbox_*helpers do not accept aper-call
base_url(they read$ACP_BASE_URLor fallback to
DEFAULT_BASE_URL). The wrapper'sbase_urlparameter is therefore a fail-fast validation on
construction; the actual HTTP base URL is configured
via the environment. This is documented in the wrapper
docstring and in the SKILL.md "Setup" section.
mainincludes PR Add antianqi/openclaw-acp-bridge v0.1.3 - peer collaboration Bridge for MiniMax Code #3's6 review rounds. The reviewer's focused diff is the
one against
07c6358(PR Add antianqi/openclaw-acp-bridge v0.1.3 - peer collaboration Bridge for MiniMax Code #3 head).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.