Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a NetworkMetrics runtime query to CLI and Rust client; implements a large AD4M plugin overhaul introducing managed vs external modes, executor/agent lifecycle and MCP session resilience, many new helper exports and tests, plugin config/schema and docs updates (executor WS port default → 12000), and CI job for plugin tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin
participant Manager as Executor Manager
participant Executor as AD4M Executor (GraphQL WS)
participant FS as File System
participant MCP as MCP Service
Plugin->>Manager: ensureExecutorRunning(token?, executorWsUrl, binaryPath)
Manager->>Manager: isExecutorRunning(executorWsUrl)?
alt not running
Manager->>FS: findExecutorBinary() / check binaryPath
Manager->>Executor: spawn executor process (binaryPath)
Executor->>Executor: start and initialize
end
Manager->>Plugin: executor ready
Plugin->>Executor: connect via GraphQL WS
Plugin->>Executor: query agent state
alt agent missing
Plugin->>Executor: generate agent (persist passphrase)
end
alt agent locked
Plugin->>Executor: unlock agent (passphrase)
end
Executor->>Plugin: return agent DID
Plugin->>MCP: mcpInitialize(authToken?)
MCP->>Plugin: sessionId
Plugin->>MCP: mcpCallTool(tool,args,sessionId)
alt HTTP 4xx (session expired)
MCP->>Plugin: 422/4xx
Plugin->>MCP: mcpInitialize(auth) -> new sessionId
Plugin->>MCP: mcpCallTool(tool,args,newSessionId)
end
MCP->>Plugin: tool result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
plugins/ad4m/openclaw.plugin.json (2)
42-45:⚠️ Potential issue | 🟠 MajorInconsistent default port for
executorWsUrl.The default value
ws://localhost:12100/graphqlconflicts with the rest of the PR which consistently uses port12000. Inplugins/ad4m/index.ts(line 899), the fallback isws://localhost:12000/graphql.🐛 Suggested fix
"executorWsUrl": { "type": "string", - "default": "ws://localhost:12100/graphql", + "default": "ws://localhost:12000/graphql", "description": "AD4M executor GraphQL WebSocket URL for waker subscriptions" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/openclaw.plugin.json` around lines 42 - 45, The default WebSocket URL for the executor (executorWsUrl) uses port 12100 which is inconsistent with the rest of the PR; change the default value string "ws://localhost:12100/graphql" to use port 12000 so it matches the fallback used in plugins/ad4m/index.ts (executorWsUrl and the fallback in the code at index.ts around the waker/executor logic).
86-89:⚠️ Potential issue | 🟡 MinorUpdate placeholder to match new default port.
The placeholder should reflect the updated default port for consistency.
📝 Suggested fix
"executorWsUrl": { "label": "Executor WebSocket URL", - "placeholder": "ws://localhost:12100/graphql" + "placeholder": "ws://localhost:12000/graphql" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/openclaw.plugin.json` around lines 86 - 89, The placeholder for the "executorWsUrl" configuration still shows the old port; update the placeholder string in the "executorWsUrl" object (label "Executor WebSocket URL") to use the new default port (e.g., change "ws://localhost:12100/graphql" to "ws://localhost:12101/graphql" or the currently agreed default) so the JSON reflects the correct default WebSocket URL.plugins/ad4m/skills/ad4m/SKILL.md (1)
246-258:⚠️ Potential issue | 🟡 MinorAdd language specifier to fenced code block.
Static analysis flagged this code block as missing a language specifier.
📝 Suggested fix
-``` +```text 1. neighbourhood_join_from_url(url: "neighbourhood://Qm...") 2. list_perspectives() → find the joined perspective UUID (the NH URL maps to a LOCAL perspective UUID) ... ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/skills/ad4m/SKILL.md` around lines 246 - 258, The fenced code block containing the numbered steps (starting with "neighbourhood_join_from_url(url: \"neighbourhood://Qm...\")" and including "list_perspectives()", "subscribe_to_mentions(perspective_id: ...)", etc.) lacks a language specifier; update the opening fence to include a language (e.g., ```text) so static analysis and renderers treat it as plain text. Ensure you only modify the opening triple backticks for that specific block and leave the block contents and closing backticks unchanged.plugins/ad4m/skills/ad4m/references/setup.md (1)
282-282:⚠️ Potential issue | 🟡 MinorInconsistent port reference in troubleshooting table.
The waker troubleshooting entry still references
ws://localhost:12100/graphqlwhile all other documentation has been updated to use port12000. This inconsistency could confuse users.📝 Suggested fix
-| Waker not firing | WS not accessible or bad query | Check `ws://localhost:12100/graphql` and waker logs | +| Waker not firing | WS not accessible or bad query | Check `ws://localhost:12000/graphql` and waker logs |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/skills/ad4m/references/setup.md` at line 282, Update the waker troubleshooting table row that currently uses the string "ws://localhost:12100/graphql" (the row beginning with "Waker not firing") to use the correct port 12000 so it reads "ws://localhost:12000/graphql"; verify the single table cell with the bad query/WS accessibility advice is updated and consistent with the other documentation entries that use port 12000.
🧹 Nitpick comments (2)
plugins/ad4m/index.test.ts (1)
569-619: Test manipulates real filesystem paths — ensure CI isolation.This test backs up and restores
~/.ad4m/mainnet_seed.seed, which could affect a real AD4M installation if tests run on a developer machine with AD4M configured. The backup/restore logic is sound, but consider documenting this behavior or using a mock filesystem in the future.Based on learnings: "Kill any lingering
ad4m-executorprocesses before running integration tests to avoid port conflicts" — similar care should be taken with filesystem state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/index.test.ts` around lines 569 - 619, The test manipulates the real user HOME (~/.ad4m/mainnet_seed.seed) which can affect a developer's AD4M install; instead, make the test fully isolated by directing code that computes seedFile to a temporary test directory or by using an in-memory/mock filesystem: modify the test to set process.env.HOME (or process.env.USERPROFILE) to a temp directory (or use mock-fs) before computing seedFile, ensure restore of env after the test, and keep the existing logic that backs up/renames and restores only within that temp directory; locate the seedFile/backupFile setup in this test, and adjust surrounding uses of ensureExecutorRunning, mockExecFileSync, mockSpawn, and createFakeChildProcess accordingly so no real ~/.ad4m files are touched.plugins/ad4m/index.ts (1)
925-929: Minor: Redundant credential storage.When
adminCredentialis retrieved from storage (line 918), it's stored again at line 927. This is harmless but unnecessary.♻️ Suggested optimization
- // Store the adminCredential we end up using - if (!providedConfig.adminCredential && adminCredential) { - storeAdminCredential(adminCredential); - } + // Store the adminCredential only if we generated a new one + if (!providedConfig.adminCredential && !storedAdminCred && adminCredential) { + storeAdminCredential(adminCredential); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/index.ts` around lines 925 - 929, The code redundantly calls storeAdminCredential(adminCredential) when adminCredential was already retrieved from storage; remove that redundant storage call and only set authToken = adminCredential here. Keep any storage of newly-created credentials at the creation site (e.g., where adminCredential is generated), and ensure you only call storeAdminCredential from that creation logic rather than in the block guarded by providedConfig.adminCredential/adminCredential.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/ad4m/index.ts`:
- Around line 56-64: The generateRandomPassphrase function uses Math.random
which is not cryptographically secure; replace it with Node.js crypto to
generate secure bytes (import { randomBytes } from 'crypto') and map those bytes
to your character set in generateRandomPassphrase so selection is uniform and
safe for credentials (or generate a URL-safe base64 string using randomBytes and
trim/pad to desired length). Update the function generateRandomPassphrase to use
randomBytes for randomness and ensure the passphrase length semantics remain the
same.
In `@plugins/ad4m/skills/ad4m/SKILL.md`:
- Around line 234-240: Update the fenced code block showing the command sequence
(the block containing "Install & configure plugin → openclaw plugins install
./ad4m" and commands like list_perspectives, set_agent_profile,
set_agent_profile_picture) to include a language specifier (e.g., "text") after
the opening ``` so the block becomes ```text; this will satisfy the linter while
keeping the displayed commands unchanged.
---
Outside diff comments:
In `@plugins/ad4m/openclaw.plugin.json`:
- Around line 42-45: The default WebSocket URL for the executor (executorWsUrl)
uses port 12100 which is inconsistent with the rest of the PR; change the
default value string "ws://localhost:12100/graphql" to use port 12000 so it
matches the fallback used in plugins/ad4m/index.ts (executorWsUrl and the
fallback in the code at index.ts around the waker/executor logic).
- Around line 86-89: The placeholder for the "executorWsUrl" configuration still
shows the old port; update the placeholder string in the "executorWsUrl" object
(label "Executor WebSocket URL") to use the new default port (e.g., change
"ws://localhost:12100/graphql" to "ws://localhost:12101/graphql" or the
currently agreed default) so the JSON reflects the correct default WebSocket
URL.
In `@plugins/ad4m/skills/ad4m/references/setup.md`:
- Line 282: Update the waker troubleshooting table row that currently uses the
string "ws://localhost:12100/graphql" (the row beginning with "Waker not
firing") to use the correct port 12000 so it reads
"ws://localhost:12000/graphql"; verify the single table cell with the bad
query/WS accessibility advice is updated and consistent with the other
documentation entries that use port 12000.
In `@plugins/ad4m/skills/ad4m/SKILL.md`:
- Around line 246-258: The fenced code block containing the numbered steps
(starting with "neighbourhood_join_from_url(url: \"neighbourhood://Qm...\")" and
including "list_perspectives()", "subscribe_to_mentions(perspective_id: ...)",
etc.) lacks a language specifier; update the opening fence to include a language
(e.g., ```text) so static analysis and renderers treat it as plain text. Ensure
you only modify the opening triple backticks for that specific block and leave
the block contents and closing backticks unchanged.
---
Nitpick comments:
In `@plugins/ad4m/index.test.ts`:
- Around line 569-619: The test manipulates the real user HOME
(~/.ad4m/mainnet_seed.seed) which can affect a developer's AD4M install;
instead, make the test fully isolated by directing code that computes seedFile
to a temporary test directory or by using an in-memory/mock filesystem: modify
the test to set process.env.HOME (or process.env.USERPROFILE) to a temp
directory (or use mock-fs) before computing seedFile, ensure restore of env
after the test, and keep the existing logic that backs up/renames and restores
only within that temp directory; locate the seedFile/backupFile setup in this
test, and adjust surrounding uses of ensureExecutorRunning, mockExecFileSync,
mockSpawn, and createFakeChildProcess accordingly so no real ~/.ad4m files are
touched.
In `@plugins/ad4m/index.ts`:
- Around line 925-929: The code redundantly calls
storeAdminCredential(adminCredential) when adminCredential was already retrieved
from storage; remove that redundant storage call and only set authToken =
adminCredential here. Keep any storage of newly-created credentials at the
creation site (e.g., where adminCredential is generated), and ensure you only
call storeAdminCredential from that creation logic rather than in the block
guarded by providedConfig.adminCredential/adminCredential.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e953f098-6729-497b-b521-efc667085d08
📒 Files selected for processing (11)
cli/src/runtime.rsplugins/ad4m/README.mdplugins/ad4m/index.test.tsplugins/ad4m/index.tsplugins/ad4m/openclaw.plugin.jsonplugins/ad4m/package.jsonplugins/ad4m/skills/ad4m/SKILL.mdplugins/ad4m/skills/ad4m/references/setup.mdplugins/ad4m/skills/ad4m/references/waker.mdrust-client/src/runtime.gqlrust-client/src/runtime.rs
Comment on lines
234
to
240
| ``` | ||
| 0. Start the ad4m-executor → see references/setup.md | ||
| 1. Install & configure plugin → openclaw plugins install @coasys/openclaw-ad4m | ||
| Set adminCredential in plugins.entries.ad4m.config | ||
| (or use JWT auth flow → see rule 3) | ||
| 2. AD4M tools are now available → list_perspectives, add_perspective, etc. | ||
| 1. Install & configure plugin → openclaw plugins install ./ad4m | ||
| (or set mode: "managed" in config) | ||
| 2. AD4M tools are now available → list_perspectives, add_perspective, etc. | ||
| 3. Create profile → set_agent_profile(username: "...") | ||
| 4. Set profile image → set_agent_profile_picture(image_base64: "...") | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language specifier to fenced code block.
Static analysis flagged this code block as missing a language specifier. Since this shows command sequences, consider adding a language identifier.
📝 Suggested fix
-```
+```text
1. Install & configure plugin → openclaw plugins install ./ad4m
(or set mode: "managed" in config)
2. AD4M tools are now available → list_perspectives, add_perspective, etc.
3. Create profile → set_agent_profile(username: "...")
4. Set profile image → set_agent_profile_picture(image_base64: "...")
```🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/ad4m/skills/ad4m/SKILL.md` around lines 234 - 240, Update the fenced
code block showing the command sequence (the block containing "Install &
configure plugin → openclaw plugins install ./ad4m" and commands like
list_perspectives, set_agent_profile, set_agent_profile_picture) to include a
language specifier (e.g., "text") after the opening ``` so the block becomes
```text; this will satisfy the linter while keeping the displayed commands
unchanged.
Replace vague 'write to your memory/notes file' with specific memory/ad4m-neighbourhoods.md file requirement. Changes: - Rule 10 now specifies exact file path and provides template - Added workflow steps (document BEFORE subscribing) - Updated join workflow to reference the specific file - Updated wake event handling to read the specific file - Added 'REQUIRED' emphasis to make it harder to skip
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ad4m/skills/ad4m/SKILL.md (1)
266-278:⚠️ Potential issue | 🟡 MinorAdd a language specifier to this fenced block.
This new command/example block is also missing a fence language, so markdownlint will keep failing on it.
📝 Suggested doc fix
-``` +```text 1. neighbourhood_join_from_url(url: "neighbourhood://Qm...") 2. list_perspectives() → find the joined perspective UUID (the NH URL maps to a LOCAL perspective UUID) 3. UPDATE memory/ad4m-neighbourhoods.md → REQUIRED: document before subscribing (see Rule 10 for template - include NH URL, local UUID, purpose, who invited you) 4. subscribe_to_mentions(perspective_id: "...") → live waker subscription (see rule 11) 5. channel_query(perspective_id: "...") → list channels 6. message_list(perspective_id, parent="<channel-id>") → returns message addresses, then message_get() per message 7. message_create(perspective_id, body="Hello!", parent="<channel-id>") → creates message AND adds to channel in one step (expression_address auto-generated)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@plugins/ad4m/skills/ad4m/SKILL.mdaround lines 266 - 278, The fenced code
block showing the numbered neighbourhood commands (starting with
"neighbourhood_join_from_url..." and listing list_perspectives, UPDATE
memory/ad4m-neighbourhoods.md, subscribe_to_mentions, channel_query,
message_list, message_create) needs a language specifier to satisfy
markdownlint; update the opening fence to include a language like "text" (e.g.flagging it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/ad4m/skills/ad4m/SKILL.md`:
- Around line 24-32: The documentation snippet in SKILL.md shows a top-level
"ad4m" key which conflicts with the actual OpenClaw placement under
plugins.entries.ad4m.config; update the example to show the correct nested shape
by removing the extra "ad4m" wrapper and showing just the config object (i.e.,
the contents of plugins.entries.ad4m.config), or alternatively present the full
OpenClaw JSON with plugins -> entries -> ad4m -> config keys; adjust the minimal
config example in SKILL.md (the JSON block currently showing "ad4m":
{"mode":"managed"}) to instead show {"mode":"managed"} as the config body or the
full nested path so copy-pasting into openclaw.json is accurate.
---
Outside diff comments:
In `@plugins/ad4m/skills/ad4m/SKILL.md`:
- Around line 266-278: The fenced code block showing the numbered neighbourhood
commands (starting with "neighbourhood_join_from_url..." and listing
list_perspectives, UPDATE memory/ad4m-neighbourhoods.md, subscribe_to_mentions,
channel_query, message_list, message_create) needs a language specifier to
satisfy markdownlint; update the opening fence to include a language like "text"
(e.g. ```text) so the block is fenced as a specific language and the linter will
stop flagging it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adb4afb8-6c9e-4fb3-8b5a-eed587b09597
📒 Files selected for processing (1)
plugins/ad4m/skills/ad4m/SKILL.md
Comment on lines
+24
to
+32
| **Minimal config (auto-generated if not provided):** | ||
|
|
||
| ```json | ||
| { | ||
| "ad4m": { | ||
| "mode": "managed" | ||
| } | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
Show the actual OpenClaw config shape here.
This snippet uses a top-level ad4m key, but Lines 55-60 say the plugin is configured under plugins.entries.ad4m.config. Copy-pasting this into openclaw.json would be misleading. Either label this as the nested config object only, or show the full config path.
📝 Suggested doc fix
-**Minimal config (auto-generated if not provided):**
+**Minimal plugin config** (`plugins.entries.ad4m.config`, auto-generated if not provided):
```json
{
- "ad4m": {
- "mode": "managed"
- }
+ "mode": "managed"
}
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
**Minimal plugin config** (`plugins.entries.ad4m.config`, auto-generated if not provided):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/ad4m/skills/ad4m/SKILL.md` around lines 24 - 32, The documentation
snippet in SKILL.md shows a top-level "ad4m" key which conflicts with the actual
OpenClaw placement under plugins.entries.ad4m.config; update the example to show
the correct nested shape by removing the extra "ad4m" wrapper and showing just
the config object (i.e., the contents of plugins.entries.ad4m.config), or
alternatively present the full OpenClaw JSON with plugins -> entries -> ad4m ->
config keys; adjust the minimal config example in SKILL.md (the JSON block
currently showing "ad4m": {"mode":"managed"}) to instead show {"mode":"managed"}
as the config body or the full nested path so copy-pasting into openclaw.json is
accurate.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ad4m/skills/ad4m/SKILL.md (1)
266-278:⚠️ Potential issue | 🟡 MinorAdd language specifier to fenced code block.
Static analysis flagged this code block as missing a language specifier. Since this shows command sequences, use
textas the language identifier.📝 Suggested fix
-``` +```text 1. neighbourhood_join_from_url(url: "neighbourhood://Qm...") 2. list_perspectives() → find the joined perspective UUID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/skills/ad4m/SKILL.md` around lines 266 - 278, The fenced code block showing the sequence of AD4M commands (including neighbourhood_join_from_url, list_perspectives, subscribe_to_mentions, channel_query, message_list, message_create) is missing a language specifier; update the opening triple-backtick for that block to include the language identifier text (i.e., ```text) so static analysis passes and the commands render correctly.
🧹 Nitpick comments (3)
plugins/ad4m/index.ts (2)
131-132: Module-level mutable state for executor process management.These module-level variables track the spawned executor process. This design works for a single plugin instance but could cause issues if the plugin is loaded multiple times. Consider documenting this limitation or encapsulating the state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/index.ts` around lines 131 - 132, The module-level mutable variables executorProcess and executorLogStream create global state that breaks if the plugin is loaded multiple times; refactor by encapsulating these into an instance-scoped manager (e.g., create an ExecutorManager class or attach fields to the plugin instance) and move any spawn/kill/logging logic to instance methods so each plugin instance has its own executorProcess and executorLogStream, or alternatively add a clear comment in plugins/ad4m/index.ts documenting the single-instance limitation if you choose not to refactor.
363-396: Dynamic requires inside function.Using
require()inside the function ensures lazy loading but may cause issues with bundlers or type checking. The imports are already available at the module level in the test file. Consider documenting why this pattern is used (to avoid load-time issues with@holochain/clienttransitive deps as noted in the waker service).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/index.ts` around lines 363 - 396, The function createWsClient uses dynamic require() calls for Ad4mClient, ApolloClient, GraphQLWsLink, createClient and ws which can break bundlers/type-checking; either hoist these imports to module-level (replace the require()s with top-level imports for Ad4mClient, ApolloClient/InMemoryCache, GraphQLWsLink, createClient and WebSocket) and remove the in-function requires, or if lazy loading is required to avoid load-time issues, add a clear comment above createWsClient explaining that dynamic requires are intentional (referencing the transitive `@holochain/client` issue) and keep the pattern; ensure wsClient and client initialization remains unchanged in createWsClient.plugins/ad4m/index.test.ts (1)
413-463: Filesystem manipulation in tests may cause flakiness.These tests manipulate the real filesystem (
~/.ad4m/mainnet_seed.seed), which can cause issues:
- Parallel test runs may conflict
- CI environments may have different permissions
- Test failures could leave the filesystem in an inconsistent state
Consider using a mock filesystem or temporary directories with unique names per test run.
💡 Alternative approach using temp directories
import os from "os"; // In test setup: const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "ad4m-test-")); // Mock HOME to point to tempDir vi.stubEnv("HOME", tempDir); // In afterEach: fs.rmSync(tempDir, { recursive: true, force: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/index.test.ts` around lines 413 - 463, The test "runs ad4m-executor init when seed file is missing" manipulates the real HOME seed file which causes flakiness; change the test to create an isolated temp home directory (use fs.mkdtempSync(path.join(os.tmpdir(), "ad4m-test-"))), stub process env HOME/USERPROFILE for the duration of the test (vi.stubEnv) so ensureExecutorRunning("cred", logger) reads a temp ~/.ad4m/mainnet_seed.seed instead of the real one, and remove the temp dir in finally/afterEach to guarantee cleanup; this eliminates the rename/backup logic and avoids touching the real filesystem while keeping the rest of the test (mockExecFileSync, mockSpawn, fakeProc, logger) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/ad4m/README.md`:
- Line 51: The README entry for the plugin field is inconsistent with the plugin
schema: change the README.md table row that currently lists `adminCredential` to
use `token` (matching the field in openclaw.plugin.json) and keep the
description (e.g., "Admin credential for the ad4m-executor"); also scan README
examples and docs for any other mentions of `adminCredential` and replace them
with `token` so docs and the `openclaw.plugin.json` schema reference the same
config key.
In `@plugins/ad4m/skills/ad4m/SKILL.md`:
- Around line 57-60: The docs reference the wrong config field name: replace any
usage of "adminCredential" with "token" in the SKILL.md description so it
matches the plugin config schema; specifically update the list entry that
currently reads `adminCredential — admin credential for the AD4M executor
(auto-generated in managed mode)` to use `token` and preserve the explanatory
text about auto-generation in managed mode and its relation to the AD4M
executor.
---
Outside diff comments:
In `@plugins/ad4m/skills/ad4m/SKILL.md`:
- Around line 266-278: The fenced code block showing the sequence of AD4M
commands (including neighbourhood_join_from_url, list_perspectives,
subscribe_to_mentions, channel_query, message_list, message_create) is missing a
language specifier; update the opening triple-backtick for that block to include
the language identifier text (i.e., ```text) so static analysis passes and the
commands render correctly.
---
Nitpick comments:
In `@plugins/ad4m/index.test.ts`:
- Around line 413-463: The test "runs ad4m-executor init when seed file is
missing" manipulates the real HOME seed file which causes flakiness; change the
test to create an isolated temp home directory (use
fs.mkdtempSync(path.join(os.tmpdir(), "ad4m-test-"))), stub process env
HOME/USERPROFILE for the duration of the test (vi.stubEnv) so
ensureExecutorRunning("cred", logger) reads a temp ~/.ad4m/mainnet_seed.seed
instead of the real one, and remove the temp dir in finally/afterEach to
guarantee cleanup; this eliminates the rename/backup logic and avoids touching
the real filesystem while keeping the rest of the test (mockExecFileSync,
mockSpawn, fakeProc, logger) intact.
In `@plugins/ad4m/index.ts`:
- Around line 131-132: The module-level mutable variables executorProcess and
executorLogStream create global state that breaks if the plugin is loaded
multiple times; refactor by encapsulating these into an instance-scoped manager
(e.g., create an ExecutorManager class or attach fields to the plugin instance)
and move any spawn/kill/logging logic to instance methods so each plugin
instance has its own executorProcess and executorLogStream, or alternatively add
a clear comment in plugins/ad4m/index.ts documenting the single-instance
limitation if you choose not to refactor.
- Around line 363-396: The function createWsClient uses dynamic require() calls
for Ad4mClient, ApolloClient, GraphQLWsLink, createClient and ws which can break
bundlers/type-checking; either hoist these imports to module-level (replace the
require()s with top-level imports for Ad4mClient, ApolloClient/InMemoryCache,
GraphQLWsLink, createClient and WebSocket) and remove the in-function requires,
or if lazy loading is required to avoid load-time issues, add a clear comment
above createWsClient explaining that dynamic requires are intentional
(referencing the transitive `@holochain/client` issue) and keep the pattern;
ensure wsClient and client initialization remains unchanged in createWsClient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9581002a-a9d5-4602-8dba-5c78f48dcc38
📒 Files selected for processing (6)
plugins/ad4m/README.mdplugins/ad4m/index.test.tsplugins/ad4m/index.tsplugins/ad4m/openclaw.plugin.jsonplugins/ad4m/skills/ad4m/SKILL.mdplugins/ad4m/skills/ad4m/references/waker.md
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/ad4m/skills/ad4m/references/waker.md
| |-------|----------|---------|-------------| | ||
| | `adminCredential` | Yes | — | Admin credential for the ad4m-executor | | ||
| | `mode` | No | `managed` | `managed` = auto-manages executor + agent, `external` = connect to existing | | ||
| | `adminCredential` | No | auto-generated | Admin credential for the ad4m-executor | |
Contributor
There was a problem hiding this comment.
Documentation inconsistency: adminCredential vs token.
The README documents adminCredential but the plugin config schema (openclaw.plugin.json) uses token. This mismatch will confuse users trying to configure the plugin.
📝 Suggested fix
-| `adminCredential` | No | auto-generated | Admin credential for the ad4m-executor |
+| `token` | No | auto-generated | Auth token — JWT in external mode, auto-generated in managed mode |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `adminCredential` | No | auto-generated | Admin credential for the ad4m-executor | | |
| | `token` | No | auto-generated | Auth token — JWT in external mode, auto-generated in managed mode | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/ad4m/README.md` at line 51, The README entry for the plugin field is
inconsistent with the plugin schema: change the README.md table row that
currently lists `adminCredential` to use `token` (matching the field in
openclaw.plugin.json) and keep the description (e.g., "Admin credential for the
ad4m-executor"); also scan README examples and docs for any other mentions of
`adminCredential` and replace them with `token` so docs and the
`openclaw.plugin.json` schema reference the same config key.
Comment on lines
+57
to
+60
| - `mode` — `"managed"` (default) or `"external"`. Managed = plugin auto-manages agent. | ||
| - `mcpEndpoint` — MCP endpoint (default: `http://localhost:3001/mcp`) | ||
| - `adminCredential` — admin credential for the AD4M executor | ||
| - `adminCredential` — admin credential for the AD4M executor (auto-generated in managed mode) | ||
| - `ad4mBinaryPath` — full path to `ad4m-executor` binary (only needed if not in PATH) |
Contributor
There was a problem hiding this comment.
Field name inconsistency: adminCredential should be token.
Line 59 references adminCredential but the plugin config schema uses token. Update to match the actual config field name.
📝 Suggested fix
- `mode` — `"managed"` (default) or `"external"`. Managed = plugin auto-manages agent.
- `mcpEndpoint` — MCP endpoint (default: `http://localhost:3001/mcp`)
-- `adminCredential` — admin credential for the AD4M executor (auto-generated in managed mode)
+- `token` — auth token (JWT in external mode, auto-generated in managed mode)
- `ad4mBinaryPath` — full path to `ad4m-executor` binary (only needed if not in PATH)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - `mode` — `"managed"` (default) or `"external"`. Managed = plugin auto-manages agent. | |
| - `mcpEndpoint` — MCP endpoint (default: `http://localhost:3001/mcp`) | |
| - `adminCredential` — admin credential for the AD4M executor | |
| - `adminCredential` — admin credential for the AD4M executor (auto-generated in managed mode) | |
| - `ad4mBinaryPath` — full path to `ad4m-executor` binary (only needed if not in PATH) | |
| - `mode` — `"managed"` (default) or `"external"`. Managed = plugin auto-manages agent. | |
| - `mcpEndpoint` — MCP endpoint (default: `http://localhost:3001/mcp`) | |
| - `token` — auth token (JWT in external mode, auto-generated in managed mode) | |
| - `ad4mBinaryPath` — full path to `ad4m-executor` binary (only needed if not in PATH) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/ad4m/skills/ad4m/SKILL.md` around lines 57 - 60, The docs reference
the wrong config field name: replace any usage of "adminCredential" with "token"
in the SKILL.md description so it matches the plugin config schema; specifically
update the list entry that currently reads `adminCredential — admin credential
for the AD4M executor (auto-generated in managed mode)` to use `token` and
preserve the explanatory text about auto-generation in managed mode and its
relation to the AD4M executor.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/ad4m/index.ts (1)
1208-1259: Consider extracting the 4xx retry logic to reduce duplication.The session recovery pattern (catch 4xx → invalidateSession → ensureSession → retry) is duplicated between
callToolWithRetry(lines 1103-1127) and here inrefreshTools. Consider extracting a helper to reduce duplication.♻️ Possible refactor
async function withSessionRetry<T>( operation: () => Promise<T>, operationName: string, ): Promise<T> { await ensureSession(); try { return await operation(); } catch (err: any) { if (err.message && /MCP HTTP 4\d\d/.test(err.message)) { logger.info(`[ad4m] Session error during ${operationName}, re-initializing...`); invalidateSession(); await ensureSession(); return await operation(); } throw err; } } // Usage in refreshTools: const tools = await withSessionRetry( () => mcpListTools(endpoint, sessionId, authToken), "tool refresh" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/index.ts` around lines 1208 - 1259, The refreshTools function duplicates the 4xx session-recovery pattern also used in callToolWithRetry; extract that pattern into a helper (e.g., withSessionRetry) that calls ensureSession, runs a passed operation (like () => mcpListTools(endpoint, sessionId, authToken) or the callTool logic), and on MCP HTTP 4xx errors logs, calls invalidateSession, re-calls ensureSession, then retries the operation once; replace the local try/catch retry blocks in refreshTools and callToolWithRetry with calls to this helper to remove duplication while keeping existing logging and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.circleci/config.yml:
- Around line 210-222: The job "plugin-ad4m-tests" is missing the common setup
step that ensures pnpm is on PATH; insert the existing reusable step "setup_env"
as the first step (before the checkout/install steps) in the "plugin-ad4m-tests"
job so that pnpm is available for the subsequent "pnpm install" and "pnpm test"
commands, mirroring how "build-and-test" and "integration-tests-cli" call
setup_env.
---
Nitpick comments:
In `@plugins/ad4m/index.ts`:
- Around line 1208-1259: The refreshTools function duplicates the 4xx
session-recovery pattern also used in callToolWithRetry; extract that pattern
into a helper (e.g., withSessionRetry) that calls ensureSession, runs a passed
operation (like () => mcpListTools(endpoint, sessionId, authToken) or the
callTool logic), and on MCP HTTP 4xx errors logs, calls invalidateSession,
re-calls ensureSession, then retries the operation once; replace the local
try/catch retry blocks in refreshTools and callToolWithRetry with calls to this
helper to remove duplication while keeping existing logging and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e4b08c4-6639-441b-b715-a49dc0f375c6
📒 Files selected for processing (3)
.circleci/config.ymlplugins/ad4m/index.test.tsplugins/ad4m/index.ts
.circleci/config.yml
Outdated
Comment on lines
+210
to
+222
| plugin-ad4m-tests: | ||
| machine: true | ||
| resource_class: coasys/marvin | ||
| steps: | ||
| - checkout | ||
| - run: | ||
| name: Install dependencies | ||
| working_directory: plugins/ad4m | ||
| command: pnpm install | ||
| - run: | ||
| name: Run tests | ||
| working_directory: plugins/ad4m | ||
| command: pnpm test |
Contributor
There was a problem hiding this comment.
Add setup_env step to ensure pnpm is available.
Other jobs in this workflow call the setup_env command before using pnpm (e.g., build-and-test at line 64, integration-tests-cli at line 199). Without it, pnpm may not be in the self-hosted runner's PATH.
🔧 Suggested fix
plugin-ad4m-tests:
machine: true
resource_class: coasys/marvin
steps:
- checkout
+ - setup_env
- run:
name: Install dependencies
working_directory: plugins/ad4m
command: pnpm install
- run:
name: Run tests
working_directory: plugins/ad4m
command: pnpm test📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| plugin-ad4m-tests: | |
| machine: true | |
| resource_class: coasys/marvin | |
| steps: | |
| - checkout | |
| - run: | |
| name: Install dependencies | |
| working_directory: plugins/ad4m | |
| command: pnpm install | |
| - run: | |
| name: Run tests | |
| working_directory: plugins/ad4m | |
| command: pnpm test | |
| plugin-ad4m-tests: | |
| machine: true | |
| resource_class: coasys/marvin | |
| steps: | |
| - checkout | |
| - setup_env | |
| - run: | |
| name: Install dependencies | |
| working_directory: plugins/ad4m | |
| command: pnpm install | |
| - run: | |
| name: Run tests | |
| working_directory: plugins/ad4m | |
| command: pnpm test |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.circleci/config.yml around lines 210 - 222, The job "plugin-ad4m-tests" is
missing the common setup step that ensures pnpm is on PATH; insert the existing
reusable step "setup_env" as the first step (before the checkout/install steps)
in the "plugin-ad4m-tests" job so that pnpm is available for the subsequent
"pnpm install" and "pnpm test" commands, mirroring how "build-and-test" and
"integration-tests-cli" call setup_env.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/ad4m/index.ts (1)
1229-1276: Consider extracting duplicated tool registration logic.The inner and outer tool registration loops (lines 1234-1245 and 1256-1267) are nearly identical. While the current code is correct, extracting this to a helper would reduce duplication.
♻️ Optional refactor to reduce duplication
+ function registerNewTools(tools: McpTool[]): number { + let newCount = 0; + for (const tool of tools) { + if (!registeredTools.has(tool.name)) { + registerMcpTool(tool); + newCount++; + } + } + if (newCount > 0) { + logger.info( + `[ad4m] Registered ${newCount} new tool(s), total: ${registeredTools.size}`, + ); + } + return newCount; + } async function refreshTools(): Promise<number> { try { await ensureSession(); try { const tools = await mcpListTools(endpoint, sessionId, authToken); - let newCount = 0; - for (const tool of tools) { - if (!registeredTools.has(tool.name)) { - registerMcpTool(tool); - newCount++; - } - } - if (newCount > 0) { - logger.info( - `[ad4m] Registered ${newCount} new tool(s), total: ${registeredTools.size}`, - ); - } - return newCount; + return registerNewTools(tools); } catch (err: any) { if (err.message && /MCP HTTP 4\d\d/.test(err.message)) { // ... session recovery ... const tools = await mcpListTools(endpoint, sessionId, authToken); - // ... duplicate loop ... + return registerNewTools(tools); } throw err; } } catch (err: any) { logger.warn(`[ad4m] Tool refresh failed: ${err.message}`); return 0; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ad4m/index.ts` around lines 1229 - 1276, The two nearly identical loops in refreshTools that iterate over the result of mcpListTools and call registerMcpTool for each unregistered tool should be extracted to a single helper to remove duplication; add a helper function (e.g., processToolList or registerNewTools) that accepts the tools array, iterates over each tool checking registeredTools.has(tool.name), calls registerMcpTool(tool) for new ones, and returns the newCount, then replace both loop blocks in refreshTools (the primary path and the retry-after-invalidateSession path) with a call to that helper and reuse the existing logging of newCount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/ad4m/index.ts`:
- Around line 1229-1276: The two nearly identical loops in refreshTools that
iterate over the result of mcpListTools and call registerMcpTool for each
unregistered tool should be extracted to a single helper to remove duplication;
add a helper function (e.g., processToolList or registerNewTools) that accepts
the tools array, iterates over each tool checking
registeredTools.has(tool.name), calls registerMcpTool(tool) for new ones, and
returns the newCount, then replace both loop blocks in refreshTools (the primary
path and the retry-after-invalidateSession path) with a call to that helper and
reuse the existing logging of newCount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 657088c1-b005-4be2-9a46-0189814e6e90
📒 Files selected for processing (2)
plugins/ad4m/index.test.tsplugins/ad4m/index.ts
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.
Make plugin work with ease
Two config modes for the plugin:
=> setup without any config required.
=>
openclaw plugins install @coasys/ad4m-openclaw-plugin& ask agent to join neighbourhood. done.Lot's of fixes
misc
network-metricscommand which I needed to debug on agent machineSummary by CodeRabbit
New Features
Documentation
Tests
Chores
Packaging