Skip to content

fix: review feedback — credentials, cross-platform paths, runtime deps - #1

Open
antianqi wants to merge 1 commit into
mainfrom
fix/review-feedback-2026-08-15
Open

fix: review feedback — credentials, cross-platform paths, runtime deps#1
antianqi wants to merge 1 commit into
mainfrom
fix/review-feedback-2026-08-15

Conversation

@antianqi

Copy link
Copy Markdown
Owner

What

Addresses the 3 review comments from 老板 (2026-08-15):

  1. Credentials disclosure contradiction — removed read_token_from_server() (which scraped the token from acp-server.py source — itself a credential leak). ACP_TOKEN is now required, no default value anywhere. Server refuses to start without it (exit 2 + stderr). Client raises ACPTokenMissing on construction. The peer prompt template now reads $ACP_TOKEN from env at call time. Server startup banner prints only length=N, never the value.

  2. Portability claim was false — introduced openclaw-skill/acp_paths.py as the single source of truth for filesystem locations. Removed every D:\\openclaw-acp and %USERPROFILE%\\.openclaw\\skills\\mavis-coding hardcoding. Default ACP_HOME = ~/.openclaw-acp on every platform. macOS, Linux, and any Windows drive letter all work. INSTALL.md "Requirements" now lists the supported-platform matrix (Win 10/11, macOS 12+, Ubuntu 22.04+, Debian 12+, Fedora 38+, Python ≥3.10).

  3. External server/SDK was an undeclared runtime dependency — pinned websockets>=16.0,<17 in requirements.txt. Documented Mavis Coding CLI subcommand contract in new docs/RUNTIME_DEPS.md. Added tests/test_smoke.py — PR-reproducible, no Mavis CLI required, runs in <10s, all 47 checks passing.

Smoke test results

[Test 1] 32/32 — no hardcoded D:\\openclaw-acp / %USERPROFILE% in any .py
[Test 2]  6/6  — acp_paths resolves cross-platform (defaults to ~/.openclaw-acp)
[Test 3]  2/2  — server refuses empty ACP_TOKEN (exit code 2 + stderr)
[Test 4-7] 8/8 — health 200, auth 401/200, inbox write/read roundtrip
[Test 8]  1/1  — server stdout does not leak the token
ALL CHECKS PASSED

Reply to each review point

① 凭据披露互相矛盾

  • Auth mode: single Bearer token, enforced on every authenticated endpoint.
  • Where the token is read from: $ACP_TOKEN env var ONLY. No default, no source-code scraping, no runtime generation, no stdin input.
  • Where it is sent:
    • HTTP → Authorization: Bearer <token> header.
    • WebSocket → ?token=<token> query parameter (browser WebSocket APIs cannot set custom headers).
  • Agent never sees the raw value:
    • server startup banner prints only length={len(AUTH_TOKEN)}.
    • client / peer prompt template / docs examples never embed a literal token.
    • peer template teaches mavis to call os.environ["ACP_TOKEN"] at request time.

② 可移植性不成立

  • Single source of truth: openclaw-skill/acp_paths.py (resolve_acp_home / resolve_openclaw_home / resolve_mcode_cmd / resolve_temp_dir / sessions_root).
  • $ACP_HOME overrides the default; default ~/.openclaw-acp on every platform.
  • INSTALL.md "Requirements" lists supported platforms explicitly: Windows 10/11, macOS 12+, Ubuntu 22.04+, Debian 12+, Fedora 38+, Python ≥3.10.
  • Smoke test Test 1 greps all 8 .py files; verified zero D:\\openclaw-acp / %USERPROFILE%\\.openclaw / %USERPROFILE%\\AppData literals.

③ 外部 server/SDK 是运行时依赖

  • requirements.txt: websockets>=16.0,<17 (v16 introduced asyncio.server API; v17+ may break).
  • docs/RUNTIME_DEPS.md: full version contract — Python 3.10+, websockets 16.x, Mavis Coding CLI subcommand contract, mavis-coding skill relationship, complete env-var table.
  • tests/test_smoke.py: PR-reproducible, 47/47 passing, does NOT require Mavis Coding CLI (uses temp DB + stub subprocess, <10s).

Side fixes (encountered while testing)

  • _acp_default_db_path() was referenced before defined in acp_store.py / acp_inbox_store.py (NameError → server exit 1). Reordered; also added missing from pathlib import Path.
  • from pathlib import Path missing in acp-server.py top.
  • Removed print(f' Auth: {AUTH_TOKEN}') from server main() (token leak).
  • PITFALLS #9 marked RESOLVED — the "scrape token from source" trick was itself a credential leak; the new model makes it impossible.

Files changed

 INSTALL.md                  | 244 ++++++++++++++++++++----
 PITFALLS.md                 |  34 +---
 README.md                   | 237 ++++++++++++++++-----
 client/acp_client.py        | 139 +++++++-----
 docs/RUNTIME_DEPS.md        | new (160 lines)
 openclaw-skill/SKILL.md     | 112 +++++++----
 openclaw-skill/acp_paths.py | new (150 lines)
 openclaw-skill/acp_peer.py  |  68 ++++---
 openclaw-skill/acp_tools.py |  93 ++++----
 requirements.txt            |   6 +-
 server/acp-server.py        |  51 +++--
 server/acp_inbox_store.py   |  15 ++-
 server/acp_store.py         |  19 ++-
 tests/test_smoke.py         | new (310 lines, 47 checks)

14 files changed, +1200 / -396

Addresses three review issues from 老板:

1. Credentials: removed read_token_from_server source-scraping; ACP_TOKEN is
   now required (no default); server refuses to start without it; client raises
   ACPTokenMissing; peer prompt template reads token from env var only; startup
   banner prints token length, never the value.

2. Portability: introduced acp_paths.py as single source of truth for all
   filesystem locations; removed all D:\\openclaw-acp and %USERPROFILE%\\.openclaw
   hardcoding; default ACP_HOME=~/.openclaw-acp on every platform; added
   Supported Platforms section to INSTALL.md (Win/macOS/Linux).

3. Runtime deps: pinned websockets>=16.0,<17; added docs/RUNTIME_DEPS.md with
   full version contract; added tests/test_smoke.py (PR-reproducible, no Mavis
   CLI required, <10s, all 47 checks passing).

Side fixes:
- _acp_default_db_path() ordering bug in acp_store / acp_inbox_store
- missing 'from pathlib import Path' in acp-server.py / acp_store.py /
  acp_inbox_store.py
- removed print(f'  Auth:  {AUTH_TOKEN}') leak from server main()
- PITFALLS #9 marked RESOLVED (source-scraping was itself a credential leak)

Smoke test: 47/47 PASS (no D:/%USERPROFILE% literals; cross-platform
path resolution; server refuses empty ACP_TOKEN with exit 2; health +
auth + inbox roundtrip; token not in stdout).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant