Conversation
Code Review ReportProject: BofAI/docs PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR updates the MCP Server Skills documentation across two commit batches: the first batch ( The documentation is generally high quality — technically precise, well-structured, and rich with concrete examples. Two major issues require attention before merge: the silent breaking change in the skills install path (no migration guidance for existing users), and a required Node.js version bump (18+ → 20+) that is inconsistently applied across documents. Four minor issues cover an undocumented token, risk-rating context gaps, a missing canonical note about the Change SummaryGroup 1 — x402 Protocol Documentation Updates
Purpose: Documents that the Group 2 — Skills Installation & Catalog Documentation Overhaul
Purpose: Reflects a renamed install layout (skills now live at Detailed FindingsMajor[MJ-01] Silent Breaking Change — Install Path Rename With No Migration Guide
Description
Code -1. Does the skill directory exist? Run `ls ~/.openclaw/skills` in your terminal.
-2. Are dependencies installed? Go to the skill folder and run `npm install` (e.g., `cd ~/.openclaw/skills/tronscan-skill && npm install`).
+1. Does the skill directory exist? Run `ls ~/.agents/skills` in your terminal.
+2. Are dependencies installed? Go to the skill folder and run `npm install` (e.g., `cd ~/.agents/skills/tronscan-data-lookup && npm install`).Recommendation Add a migration callout block near the top of :::caution Upgrading from a previous installation?
The skills directory has moved from `~/.openclaw/skills/` to `~/.agents/skills/`.
If you already have skills installed, migrate them with:
```bash
mkdir -p ~/.agents/skills
mv ~/.openclaw/skills/* ~/.agents/skills/Then re-run Recommendation Add a Prerequisites or Requirements section at the top of :::note Prerequisites
- **Node.js 20 or higher** — required by `agent-wallet`, `x402-payment`, and related signing skills.
Run `node --version` to check. If needed, upgrade via [nvm](https://github.com/nvm-sh/nvm) or the Node.js installer.
:::Also update Minor[MN-01]
|
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | docs/x402/core-concepts/network-and-token-support.md : Lines 533–534; docs/x402/faq.md : Network table |
Description
A new token
EPSoneip155:56(BSC Mainnet) is added to the token support table with contract address0xA7f552078dcC247C2684336020c03648500C6d9F. Unlike USDC and DHLU, which each receive at least a parenthetical explanation in adjacent prose,EPSis listed with no description, no link, and no explanation of what protocol or project it belongs to. Users unfamiliar with DeFi may not recognise "EPS" and cannot make an informed decision about using it for payments.
Code
+| **EPS** | `eip155:56` | `0xA7f552078dcC247C2684336020c03648500C6d9F` |Recommendation
Add a brief inline note, e.g.:
| **EPS** | `eip155:56` | `0xA7f552...` | Ellipsis Protocol stablecoin (BEP-20); uses `exact_permit` scheme |Or add a sentence in the surrounding prose explaining the token's origin and payment scheme compatibility.
[MN-02] Security Risk Ratings Displayed Without Explanation or Guidance
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | docs/McpServer-Skills/SKILLS/QuickStart.md : Security scan table (step 6) |
Description
The updated security scan table now shows the new
bankofai-guideskill with "Med Risk" (Gen) and "High Risk" (Snyk). The previous table hadagent-walletat "Med Risk / 1 alert / High Risk"; this PR changesagent-walletto "Safe / 0 alerts / High Risk" (an improvement), butbankofai-guideinherits the elevated risk ratings with no explanation.Users are told to "Review them and select Yes to proceed" but receive no guidance on what "High Risk (Snyk)" means in practice, why they should still proceed, or what the known Snyk findings are. This is especially important since
bankofai-guidestores a generated password to~/.agent-wallet/runtime_secrets.json— a security-sensitive path. A detail link is provided (https://skills.sh/BofAI/skills), but it is not mentioned in the surrounding prose.
Code
+│ bankofai-guide Med Risk 1 alert High Risk │
...
+│ Details: https://skills.sh/BofAI/skillsRecommendation
Add a prose note beneath the security table:
> **What do these ratings mean?** "High Risk (Snyk)" typically reflects transitive dependency advisories,
> not vulnerabilities in the skill's own code. Review the full details at the link above before proceeding.
> If you are in a high-security environment, audit the Snyk findings manually before installing.[MN-03] Symlink Installation Behavior and Security Implications Underdocumented
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation / Security |
| File | docs/McpServer-Skills/SKILLS/QuickStart.md : Step 5 (Installation Summary) |
Description
The PR introduces a new installation plan step documenting that Claude Code receives symlinks pointing to the universal
~/.agents/skills/copies, while all other tools receive full copies. The tip box explains the "one source of truth" benefit, but it does not address the security or operational implications:
- If a skill package is updated (e.g., via
npm install) in the universal directory, the symlinked Claude Code version is affected immediately and silently — without any re-confirmation step.- If the universal directory is deleted or moved, Claude Code's symlinks become dangling (broken) and will fail silently until the user re-runs the installer.
- Users with restricted-permission setups (e.g., some corporate environments) may see symlink creation fail without a clear error.
Code
│ ~/.agents/skills/agent-wallet │
│ universal: Antigravity, Cursor, Gemini CLI, GitHub Copilot, Amp +7 more │
│ symlink → Claude Code │
Recommendation
Expand the universal vs symlink tip box:
:::tip universal vs symlink
Tools that follow the generic skills layout get a **universal** copy under `~/.agents/skills/`.
Claude Code uses its own convention, so the installer creates a **symlink** pointing back to the
universal copy — one source of truth, both places stay in sync.
**Keep in mind:** Because of the symlink, any `npm install` or file update inside the universal
directory is instantly reflected in Claude Code — no re-installation needed, but also no review gate.
If you delete `~/.agents/skills/`, Claude Code's symlinks will break until you reinstall.
:::[MN-04] bankofai-guide Password Storage Path Lacks Explicit Security Warning
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Security / Documentation |
| File | docs/McpServer-Skills/SKILLS/BANKOFAISkill.md : bankofai-guide section (caution block) |
Description
The caution block correctly advises users to back up the auto-generated password from
~/.agent-wallet/runtime_secrets.json. However, it does not warn that this file is stored in plaintext on disk (implied by context), that its default permissions may allow other local users to read it, or that users should restrict permissions (chmod 600). For a file that controls access to wallets holding real on-chain assets, this gap is meaningful.
Code
:::caution Your password matters
The quick setup auto-generates a strong password and stores it in `~/.agent-wallet/runtime_secrets.json`
for convenience. Save or memorize it anyway — if that file is ever deleted, the password is the only
way to recover access to the encrypted wallet.
:::Recommendation
Extend the caution block:
:::caution Your password matters
The quick setup auto-generates a strong password and stores it in `~/.agent-wallet/runtime_secrets.json`
for convenience. **This file is stored in plaintext** — verify its permissions with:
```bash
ls -l ~/.agent-wallet/runtime_secrets.json
# Should show: -rw------- (600)
# If not, run: chmod 600 ~/.agent-wallet/runtime_secrets.jsonSave or memorize the password separately — if this file is deleted, the password is the only way
to recover access to your encrypted wallet.
:::
---
### Suggestions
#### [S-01] Clarify That the x402 "EPS" Token Uses `exact_permit`, Not `exact`
**File:** `docs/x402/core-concepts/network-and-token-support.md`
**Description:** The new `exact` scheme warning block explicitly states BSC USDT is not ERC-3009 compliant and must use `exact_permit`. This same guidance should be extended to EPS to make it clear which scheme each token supports, especially since users may not be familiar with EPS.
**Suggestion:** Add a brief table column "Supported Scheme" to the BSC token entries, or add a similar warning note for EPS near its table row.
---
#### [S-02] Consider a Combined "What Changed in This Release" Migration Note
**File:** `docs/McpServer-Skills/SKILLS/QuickStart.md` (or a new `CHANGELOG.md` / `migration.md`)
**Description:** This PR encompasses several user-facing breaking changes at once: install path rename, skill count increase (8→11), skill folder rename (e.g., `sunswap` → `sunswap-dex-trading`), and Node.js version bump. Surfacing these together in a "What's new / breaking changes" section at the top of the QuickStart makes it easy for returning users to know what they need to update without reading the entire document.
**Suggestion:** Add a collapsible `<details>` block at the top of QuickStart:
```markdown
<details>
<summary>⬆️ Upgrading from a previous version? See what changed</summary>
- Install path: `~/.openclaw/skills/` → `~/.agents/skills/`
- Skill count: 8 → 11 (new: `bankofai-guide`, `trx-staking-sr-voting`, `usdd-just-protocol`)
- Skill folder names: e.g., `sunswap` → `sunswap-dex-trading` (full name)
- Node.js: minimum is now **v20** (was v18)
- 2 new installer steps: Installation Plan (step 5) before Security Assessment (step 6)
</details>
[S-03] Add a --network Flag Cross-Reference to the x402-payment Skill Section
File: docs/McpServer-Skills/SKILLS/BANKOFAISkill.md and Intro.md
Description: The intro's x402 section mentions "just switch the --network parameter" to change chains, and the skill section now explains TRON and BSC multi-chain support. However, neither section links to the full list of supported networks and their exact --network values.
Suggestion: Add a one-line link:
> See [Network & Token Support](../../x402/core-concepts/network-and-token-support.md) for the full
> list of supported network identifiers and tokens.Positive Observations
| Area | Observation |
|---|---|
| Technical Precision | The x402 exact scheme documentation is exceptionally detailed — correctly distinguishing ERC-3009 vs. EIP-2612 requirements, specifying which tokens use which schemes, and explaining the v2 migration fallback dual-write strategy. |
| Security UX | The bankofai-guide description correctly restricts dangerous wallet operations (remove, reset, change-password) to the terminal only and explicitly tells users the AI will explain — not execute — those commands. |
| Localization Parity | All English documentation changes are mirrored identically in the i18n/zh-Hans Chinese localization. No English-only updates were left unsynced. |
| Safety Defaults | The sunperp skill documentation now precisely quantifies the mandatory stop-loss behavior (default 5%, max 25%) instead of the vague "losses exceed 5% the AI closes your position." This is a meaningful accuracy improvement for users making financial decisions. |
| Installation UX | The new Installation Summary step (showing exactly where each skill will land and which tools get symlinks vs. full copies) significantly improves transparency before the user commits to installation. |
| Installer Warning | The updated "Done!" output now appends "Review skills before use; they run with full agent permissions." — a clear, appropriate security reminder surfaced at the right moment. |
| Brand Consistency | The "BankOfAI" → "BANK OF AI" rename is consistently applied across both English and Chinese docs. |
| V3 Fee Tier Guidance | The new SunSwap V3 tip box clearly specifies the four valid fee tiers and tick-spacing alignment rules, preventing users from submitting transactions that would fail on-chain. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 6 | 4 | 2 | 0 | MJ-01 (path migration), MJ-02 (Node.js version inconsistency) |
| Security | 8 | 6 | 2 | 0 | MN-02 (risk rating context), MN-04 (password file plaintext warning) |
| Performance | 7 | 0 | 0 | 7 | Not applicable — documentation only |
| Code Quality | 8 | 7 | 1 | 0 | MN-01 (EPS token undocumented) |
| Testing | 7 | 0 | 0 | 7 | Not applicable — documentation only |
| Documentation | 6 | 4 | 2 | 0 | MJ-01 (no migration guide), MN-03 (symlink implications) |
| Compatibility | 5 | 3 | 2 | 0 | MJ-01 (existing users), MJ-02 (Node.js 18 users) |
| Observability | 4 | 0 | 0 | 4 | Not applicable — documentation only |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analyzed only the diff between origin/main and origin/update-mcp-server. Runtime behavior, integration testing, and deployment impact are not covered. All findings pertain exclusively to documentation accuracy, completeness, and user-safety concerns — no application source code was present in this diff.
Report generated by Code Review Skill v1.0.0
Date: 2026-04-22
2 participants
No description provided.