fix: review feedback — credentials, portability, smoke test - #14
Closed
antianqi wants to merge 2 commits into
Closed
fix: review feedback — credentials, portability, smoke test#14antianqi wants to merge 2 commits into
antianqi wants to merge 2 commits into
Conversation
…(v0.1.0)
Bridge MiniMax Code to the OpenClaw-mcode-ACP server for true
peer-to-peer collaboration via the inbox protocol.
Skills:
- acp-collab : read inbox, push progress, ask/answer blocking Q
- acp-task-dispatch : fire-and-forget task dispatch with persistence
Includes:
- plugin.json with schema URL, license (Apache-2.0), author, repo
- README.md with problem statement, example prompt, requirements,
test evidence
- LICENSE (Apache-2.0, copied from repo root)
- 2 SKILL.md files (no TODO placeholders, valid frontmatter)
Validated locally: OK plugin antianqi/openclaw-acp-bridge
(5/5 inbox endpoint tests + stub-mavis end-to-end demo on the server side)
Addresses 3 review comments on PR hetaoBackend#2 (openclaw-acp-bridge v0.1.0): 1. Credentials disclosure: removed 'auth token is invalid' wording in acp-task-dispatch SKILL.md failure handling. The Plugin does not own credentials — user is now told to verify server reachability and their own environment configuration. 2. Portability: replaced hardcoded 'sys.path.insert(0, r''D:/openclaw-acp/openclaw-skill'')' in BOTH skills with ACP_HOME-driven import. Missing env var now raises a clear RuntimeError with setup instructions, instead of silently failing on macOS / Linux / non-D: Windows installs. 3. Runtime deps + smoke test: - README Requirements pinned to OpenClaw-mcode-ACP v7-bidir+ - Added scripts/smoke.py (PR-reproducible, no MiniMax Code required): validates ACP_HOME resolution, SDK import, /acp/health 200, inbox write/read roundtrip, and that no SKILL.md still references hardcoded D:/openclaw-acp paths. - Version bumped to 0.1.1 Files: README.md | 18 +- plugin.json | 2 +- scripts/smoke.py | 187 + (new) skills/acp-collab/SKILL.md | 11 +- skills/acp-task-dispatch/SKILL.md | 12 +-
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
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.
What
Addresses the 3 review comments on PR #2 (
Add antianqi/openclaw-acp-bridge v0.1.0):Credentials disclosure contradiction —
acp-task-dispatch/SKILL.mdFailure handlingused to say "the server is likely down or the auth token is invalid". Reworded to
"the server is likely down or rejected the request", and explicitly notes this
Plugin does not embed or manage credentials —
$ACP_TOKENlives in the user'senvironment. The PR's "no credentials" claim is now consistent.
Portability (ACP_HOME override was fictional) — both
SKILL.mdfiles used tocontain
sys.path.insert(0, r'D:/openclaw-acp/openclaw-skill')(literal hardcodedpath). Now they read
$ACP_HOMEfirst and raise a clearRuntimeErrorwith setupinstructions if it is unset. macOS / Linux / non-D: Windows installs now work.
External server/SDK is a runtime dependency — needs version pin + smoke test —
scripts/smoke.py(PR-reproducible, no MiniMax Code required, ~10s):validates
$ACP_HOMEresolves, SDK imports,/acp/healthreturns 200, inboxwrite/read roundtrip works, and that no
SKILL.mdstill references hardcodedD:/openclaw-acp.Reply to each review point
① 凭据披露互相矛盾
$ACP_TOKEN)里,Plugin 只发起请求、不知道也不关心凭据本身。② 可移植性不成立
sys.path.insert(0, r'D:/openclaw-acp/openclaw-skill')已替换为:ACP_HOME不设就明确报错而不是默默失败。scripts/smoke.pyCheck 6 静态扫描所有SKILL.md文件,确认没有 hardcoded 路径漏网。③ 外部 server/SDK 是运行时依赖
scripts/smoke.py:在 PR 内可复现,6 项 check:ACP_HOME 解析 / SDK 可导入 / acp_paths 跨平台 / /acp/health 200 / inbox 读写 roundtrip / SKILL.md 无 hardcoded 路径。Files changed
5 files changed, +222 / -8