openclaw: patch plugins.allow at install time + drop blocking auto-recall#124
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements issue ChangesOpenClaw allowlist patching and plugin hook simplification
Node imports and path infrastructure refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 1 file changed
Generated for commit 45e40c7. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openclaw/src/index.ts (1)
1009-1032:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
before_agent_startshould no longer be gated byautoRecall(Line 1009).Now that proactive recall was removed, this gate unintentionally disables the login nudge + post-auth welcome paths for users who already have
autoRecall: false.Suggested fix
- if (config.autoRecall !== false) { - hook("before_agent_start", async (event: { prompt?: string }) => { + hook("before_agent_start", async (event: { prompt?: string }) => { if (!event.prompt || event.prompt.length < 5) return; try { const dl = await getApi(); @@ } catch (err) { logger.error(`before_agent_start failed: ${err instanceof Error ? err.message : String(err)}`); } }); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openclaw/src/index.ts` around lines 1009 - 1032, The before_agent_start hook is incorrectly wrapped by the config.autoRecall check which prevents the login nudge and post-auth welcome for users with autoRecall: false; remove this gating so the hook registration always runs (i.e., register the hook unconditionally instead of inside the if (config.autoRecall !== false) block) while keeping the existing logic that checks getApi(), authUrl, and justAuthenticated handling inside the hook callback (referencing the before_agent_start hook, getApi(), justAuthenticated, loadCredentials(), and authUrl).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bundle/cli.js`:
- Around line 456-458: The current coercion of tools.alsoAllow to [] forces
patching even when the field is absent; change the logic so you preserve
"missing" vs "explicit empty" by not defaulting to [], e.g. keep alsoAllowRaw as
undefined when tools.alsoAllow is not an array and compute
toolsAlsoAllowNeedsPatch only when an actual array was provided (e.g.
toolsAlsoAllowNeedsPatch = Array.isArray(alsoAllowRaw) ?
!isAllowlistCoveringHivemind(alsoAllowRaw) : false); apply the same pattern to
the analogous block around lines 470-474 so patching only occurs for an
explicitly supplied non-empty allowlist.
- Line 3313: The command string built for Hermes is not quoting the executable
path which will break on paths with spaces; update the assignment that creates
command (currently using `node ${join8(BUNDLE_DIR, bundleFile)}`) to wrap the
resolved path in quotes (e.g. `node "..."`) so the `join8(BUNDLE_DIR,
bundleFile)` result is quoted, ensuring paths with spaces are handled correctly;
locate the `command` property assignment in bundle/cli.js and modify it to
include the quotes around the template substitution.
- Around line 516-529: The installer currently ignores result.status === "error"
from ensureHivemindAllowlisted(), so surface failures by handling that branch:
when ensureHivemindAllowlisted() returns result.status === "error", log a clear
error including result.error, result.configPath and result.backupPath (like the
other branches), and ensure the installer returns non-zero/aborts or otherwise
indicates failure (same behavior as on other fatal install errors). Update the
code path around ensureHivemindAllowlisted() to check result.status for "error"
and emit a descriptive processLogger/log error message using those result
fields.
- Around line 453-456: The code dereferences parsed.plugins and parsed.tools
without validating parsed's shape; update the top of the parsing logic to verify
parsed is a non-null object and throw a clear, structured error if not (e.g.,
"Invalid config: expected JSON object"); then guard access to parsed.plugins and
parsed.tools by treating them as objects (e.g., if parsed.plugins is not an
object default to {} before reading plugins.allow) and ensure tools.alsoAllow is
read only after confirming tools is an object (or default to {}), keeping the
existing assignments to pluginsAllowRaw and alsoAllowRaw but using these
validated/defaulted objects.
In `@openclaw/src/index.ts`:
- Line 663: The manual-fix string returned when updating the allowlist (the
returned object that uses result.error and result.configPath) is too
prescriptive; change it to recommend adding "hivemind" to tools' alsoAllow and
only recommend adding it to plugins.allow if an explicit plugin allowlist
already exists (i.e., plugins.allow is present and non-empty). Update the
message built around result.error/result.configPath to conditionally instruct:
if plugins.allow exists suggest adding to both, otherwise suggest adding to
tools.alsoAllow and warn not to create a plugins.allow (to avoid switching to
restrictive explicit-allow mode).
In `@openclaw/tests/auto-recall.test.ts`:
- Line 157: The test calls expect(before({ prompt: "anything that triggered the
path before" })) but uses the wrong matcher; replace .resolves.not.toThrow()
with .resolves.toBeUndefined() so the promise resolution is asserted correctly
(i.e., expect(before(...)).resolves.toBeUndefined()).
---
Outside diff comments:
In `@openclaw/src/index.ts`:
- Around line 1009-1032: The before_agent_start hook is incorrectly wrapped by
the config.autoRecall check which prevents the login nudge and post-auth welcome
for users with autoRecall: false; remove this gating so the hook registration
always runs (i.e., register the hook unconditionally instead of inside the if
(config.autoRecall !== false) block) while keeping the existing logic that
checks getApi(), authUrl, and justAuthenticated handling inside the hook
callback (referencing the before_agent_start hook, getApi(), justAuthenticated,
loadCredentials(), and authUrl).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 609726ec-8d39-46ec-ae3f-586579147896
📒 Files selected for processing (7)
bundle/cli.jsopenclaw/src/index.tsopenclaw/src/setup-config.tsopenclaw/tests/auto-recall.test.tsopenclaw/tests/install-openclaw.test.tsopenclaw/tests/setup-command.test.tssrc/cli/install-openclaw.ts
User-flow E2E: drove the real
|
429b67a to
a1ca705
Compare
Rebased onto current
|
CodeRabbit caught 5 actionable issues. All addressed.
1. before_agent_start was wrongly gated by config.autoRecall
The hook handler no longer does any recall — but the registration
block was still wrapped in `if (config.autoRecall !== false)`. For
users with autoRecall: false, that silently disabled the login URL
nudge AND the post-auth welcome banner. Removed the gate: the hook
always registers; the body only does auth-state work which is
unrelated to recall.
2. tools.alsoAllow coercion to [] was patching default-allow setups
The original logic at `Array.isArray(tools.alsoAllow) ? ... : []`
treated an absent alsoAllow as an empty array and then always
patched it to ["hivemind"], flipping the user from default-allow
into restrictive explicit-allowlist mode. Now matches the same
explicit-non-empty-only contract used for plugins.allow: only
patch when the user has opted into an explicit list. Absent /
empty → leave alone.
Tests updated:
- was: "handles config where alsoAllow is missing entirely"
expecting ["hivemind"] to be created
- now: "does NOT create alsoAllow when missing" expecting it to
stay undefined; plus a new case for empty-array
3. JSON.parse can return null; properties dereferenced unsafely
Added a shape check after JSON.parse in both ensureHivemindAllowlisted
and detectAllowlistMissing. JSON.parse("null") returns null, and
JSON.parse("[...]") returns an array — both would crash on .plugins
or .tools access. Now returns a structured "openclaw config is not a
JSON object" error.
Added a test for the malformed-JSON path so we don't regress.
4. installer was silent on patch-write failure
installOpenclaw() ignored `result.status === "error"`. If the
allowlist patch fails for a non-no-op reason (malformed JSON,
unwritable file), the user wouldn't know why the plugin doesn't
load. Now: keep the silent path for the common "config file not
found" case (gateway never started), but surface any other error
via warn(). CodeRabbit's exact suggestion.
5. Manual-fix error text was too strict
The slash-command failure message said "add hivemind to BOTH
arrays", which could mislead users with default-allow configs
into creating explicit allowlists. Reworded to make conditional
on each array existing as a non-empty list.
6. Vitest matcher pattern in auto-recall.test.ts
`.resolves.not.toThrow()` is invalid when the promise resolves
to `undefined` (not a function). Switched to
`.resolves.toBeUndefined()` which actually asserts the resolved
value and naturally surfaces any rejection.
Test: 2404/2404 passing. Typecheck clean. Bundle regenerated.
|
Thanks @coderabbitai — addressed 5 of the 6 findings on the latest push (7451e75 + bundle regen on 3b2f6cc). One left as intentional. Fixed:
Not changed:
2404/2404 tests pass after the changes. Typecheck clean. Bundle regenerated. |
|
On the Hermes path quoting — your reasoning is sound; it's a pre-existing concern in the install-hermes path, unrelated to this PR's surface. Would you like me to open a GitHub issue against I'll go ahead and resolve my addressed comments. (ノ◕ヮ◕)ノ*:・゚✧ ✅ Actions performedComments resolved. Approval is disabled; enable |
CodeRabbit caught 5 actionable issues. All addressed.
1. before_agent_start was wrongly gated by config.autoRecall
The hook handler no longer does any recall — but the registration
block was still wrapped in `if (config.autoRecall !== false)`. For
users with autoRecall: false, that silently disabled the login URL
nudge AND the post-auth welcome banner. Removed the gate: the hook
always registers; the body only does auth-state work which is
unrelated to recall.
2. tools.alsoAllow coercion to [] was patching default-allow setups
The original logic at `Array.isArray(tools.alsoAllow) ? ... : []`
treated an absent alsoAllow as an empty array and then always
patched it to ["hivemind"], flipping the user from default-allow
into restrictive explicit-allowlist mode. Now matches the same
explicit-non-empty-only contract used for plugins.allow: only
patch when the user has opted into an explicit list. Absent /
empty → leave alone.
Tests updated:
- was: "handles config where alsoAllow is missing entirely"
expecting ["hivemind"] to be created
- now: "does NOT create alsoAllow when missing" expecting it to
stay undefined; plus a new case for empty-array
3. JSON.parse can return null; properties dereferenced unsafely
Added a shape check after JSON.parse in both ensureHivemindAllowlisted
and detectAllowlistMissing. JSON.parse("null") returns null, and
JSON.parse("[...]") returns an array — both would crash on .plugins
or .tools access. Now returns a structured "openclaw config is not a
JSON object" error.
Added a test for the malformed-JSON path so we don't regress.
4. installer was silent on patch-write failure
installOpenclaw() ignored `result.status === "error"`. If the
allowlist patch fails for a non-no-op reason (malformed JSON,
unwritable file), the user wouldn't know why the plugin doesn't
load. Now: keep the silent path for the common "config file not
found" case (gateway never started), but surface any other error
via warn(). CodeRabbit's exact suggestion.
5. Manual-fix error text was too strict
The slash-command failure message said "add hivemind to BOTH
arrays", which could mislead users with default-allow configs
into creating explicit allowlists. Reworded to make conditional
on each array existing as a non-empty list.
6. Vitest matcher pattern in auto-recall.test.ts
`.resolves.not.toThrow()` is invalid when the promise resolves
to `undefined` (not a function). Switched to
`.resolves.toBeUndefined()` which actually asserts the resolved
value and naturally surfaces any rejection.
Test: 2404/2404 passing. Typecheck clean. Bundle regenerated.
7451e75 to
7a951f9
Compare
| const grepParams: GrepMatchParams = { | ||
| pattern: keywords.join(" "), | ||
| ignoreCase: true, | ||
| wordMatch: false, | ||
| filesOnly: false, | ||
| countOnly: false, | ||
| lineNumber: false, | ||
| invertMatch: false, | ||
| fixedString: true, | ||
| }; | ||
| const searchOpts = buildGrepSearchOptions(grepParams, "/"); | ||
| searchOpts.limit = 10; | ||
| const rows = await searchDeeplakeTables(dl, memoryTable, sessionsTable, searchOpts); | ||
| if (!rows.length) return; | ||
|
|
||
| const recalled = rows | ||
| .map(r => { | ||
| const body = normalizeContent(r.path, r.content); |
There was a problem hiding this comment.
@efenocchi yes — the entire before_agent_start recall block is gone. The hook handler now only runs two narrow paths: the login URL nudge (when no creds) and the post-auth welcome banner (once after device-flow). No more extractKeywords, no more searchDeeplakeTables call, no more <recalled-memories> injection.
Recall is now agent-initiated via the registered tools (hivemind_search, hivemind_read, hivemind_index) — matches how CC/Codex/Cursor/Hermes/Pi already work (their Grep PreToolUse intercept is the same pattern: lazy, on agent demand). The full diff for the removal is at openclaw/src/index.ts:991-1031 on the latest commit.
Note: CodeRabbit also caught (and I fixed) that I'd left the entire hook wrapped in if (config.autoRecall !== false) — that would have disabled the auth nudge + welcome banner for users with autoRecall: false. Now unconditional.
…call Two related fixes for openclaw users (issue #121): 1. plugins.allow gating: the installer drops files into ~/.openclaw/extensions/hivemind/ but never touches the gateway's openclaw.json. If plugins.allow is an explicit array missing "hivemind", openclaw refuses to load the plugin — auto-capture silently does nothing because agent_end never fires, and /hivemind_setup is unreachable from inside the agent (chicken-and-egg: the slash command needs the plugin to load). src/cli/install-openclaw.ts now calls the same ensureHivemindAllowlisted helper that /hivemind_setup uses, so the installer patches plugins.allow + tools.alsoAllow at install time (when each is an explicit array). Mirrors openclaw's own ensurePluginAllowlisted semantics: never creates the array if absent, never touches it if empty — default-allow stays default-allow so we don't flip the user into restrictive mode and break other plugins. Adds a restart hint + "capture starts on the next turn — no backfill" caveat to installer output so users aren't confused when older turns don't appear. 2. blocking auto-recall removed: before_agent_start used to run a searchDeeplakeTables query across memory + sessions on every user turn. With Deeplake's sessions-table latency varying from 200ms to 10s+ (measured 11.7s/10.8s/11.2s across three sequential probes), every openclaw turn was paying up to 10s of blocking recall before the agent could even read the message. Other agents (claude-code/codex/cursor/hermes/pi) don't do this — they intercept the agent's own Grep tool calls and let the agent decide when to search. openclaw now matches that pattern: recall flows exclusively through the registered hivemind_search/_read/_index tools, with the SKILL.md body in the system prompt directing the agent to call them. Side effect: the recursive <recalled-memories> capture (the old recall context was getting captured as part of the user's prompt and stored) is gone too. The before_agent_start hook still handles the login nudge and the post-auth welcome banner — those don't touch Deeplake. Tests: - openclaw/tests/setup-command.test.ts: 9 new cases covering plugins.allow patching, idempotency, default-allow semantics, both-arrays patched, detectAllowlistMissing extension - openclaw/tests/install-openclaw.test.ts (new): 12 cases driving the real installer against tmpdir fixtures — file copy, both patches, idempotency, default-allow preservation, malformed config tolerance, restart hint, no-duplicate - openclaw/tests/auto-recall.test.ts: rewritten — asserts no Deeplake call on a normal turn, no <recalled-memories> injection, hivemind_search/_read/_index tools still registered, login nudge path preserved Real-world E2E on a live openclaw gateway: - Bundle installed → "Hivemind plugin registered" in gateway log - Idempotency: re-running installer logs "allowlist already covers hivemind", no extra backup - Zero "Auto-recall failed" log lines after the restart with the new bundle (previous bundle emitted them on every turn that hit Deeplake slowness) Caveat: capture (agent_end → INSERT into sessions) still hits the same Deeplake-side timeout intermittently. That's a separate performance issue and runs after the agent responds, so it doesn't block the user — but it does drop some turns from memory until Deeplake's sessions queries are faster. Confidence: high, because the unit tests cover every edge case of the new config-patching logic, the local fixture E2E drove the real installer end-to-end against multiple fixture shapes, and the live gateway confirms the bundle loads + the recall hook no longer fires. Untested: openclaw versions older than 2026.4.21 (where the auto-promote-via-tools.alsoAllow heuristic doesn't exist) — the manual fix from the original reporter implies they would have been the broken-state case; the installer's plugins.allow patch is the right fix for them, but we couldn't reproduce that exact host state on the 2026.4.21 gateway available for testing. Fixes #121
CI 'Verify bundle/ directories are up to date' check failed on 7e0cccf because the previous import-path fix in tests/openclaw/setup-command.test.ts changed module-graph shape enough to renumber esbuild's deduped imports (readFileSync16 → readFileSync17, existsSync21 → existsSync22, etc.). The cli.js bundle pulls in transitively-related code so its hashed identifiers shift. Source-only commits don't pass the bundle-drift check because the release pipeline ships pre-built artifacts. Catch and commit them together going forward (or rebuild from the repo root, not a deeper worktree, where esbuild's path comments differ too). No code-behavior changes — just regenerated artifact.
CodeRabbit caught 5 actionable issues. All addressed.
1. before_agent_start was wrongly gated by config.autoRecall
The hook handler no longer does any recall — but the registration
block was still wrapped in `if (config.autoRecall !== false)`. For
users with autoRecall: false, that silently disabled the login URL
nudge AND the post-auth welcome banner. Removed the gate: the hook
always registers; the body only does auth-state work which is
unrelated to recall.
2. tools.alsoAllow coercion to [] was patching default-allow setups
The original logic at `Array.isArray(tools.alsoAllow) ? ... : []`
treated an absent alsoAllow as an empty array and then always
patched it to ["hivemind"], flipping the user from default-allow
into restrictive explicit-allowlist mode. Now matches the same
explicit-non-empty-only contract used for plugins.allow: only
patch when the user has opted into an explicit list. Absent /
empty → leave alone.
Tests updated:
- was: "handles config where alsoAllow is missing entirely"
expecting ["hivemind"] to be created
- now: "does NOT create alsoAllow when missing" expecting it to
stay undefined; plus a new case for empty-array
3. JSON.parse can return null; properties dereferenced unsafely
Added a shape check after JSON.parse in both ensureHivemindAllowlisted
and detectAllowlistMissing. JSON.parse("null") returns null, and
JSON.parse("[...]") returns an array — both would crash on .plugins
or .tools access. Now returns a structured "openclaw config is not a
JSON object" error.
Added a test for the malformed-JSON path so we don't regress.
4. installer was silent on patch-write failure
installOpenclaw() ignored `result.status === "error"`. If the
allowlist patch fails for a non-no-op reason (malformed JSON,
unwritable file), the user wouldn't know why the plugin doesn't
load. Now: keep the silent path for the common "config file not
found" case (gateway never started), but surface any other error
via warn(). CodeRabbit's exact suggestion.
5. Manual-fix error text was too strict
The slash-command failure message said "add hivemind to BOTH
arrays", which could mislead users with default-allow configs
into creating explicit allowlists. Reworded to make conditional
on each array existing as a non-empty list.
6. Vitest matcher pattern in auto-recall.test.ts
`.resolves.not.toThrow()` is invalid when the promise resolves
to `undefined` (not a function). Switched to
`.resolves.toBeUndefined()` which actually asserts the resolved
value and naturally surfaces any rejection.
Test: 2404/2404 passing. Typecheck clean. Bundle regenerated.
The rebase used `--theirs` to skip the merge of an old-source-vs-new-source bundle/cli.js conflict (the generated artifact differs because the source rebased cleanly). `npm run build` regenerates the correct artifact from the rebased source.
7a951f9 to
ad96d8b
Compare
Fixes #121.
Why
Two related bugs surfaced in a user report (the original symptom: "openclaw plugin installed but auto-capture silently does nothing"):
plugins.allowgating — the installer copies files into~/.openclaw/extensions/hivemind/but never patches~/.openclaw/openclaw.json. If the gateway'splugins.allowis an explicit array missing"hivemind", the plugin never registers,agent_endnever fires, and/hivemind_setupis unreachable from inside the agent (chicken-and-egg: the slash command needs the plugin to load first).before_agent_startran a Deeplakesessions-table search on every user turn. Sessions queries currently take 11.7s / 10.8s / 11.2s (three sequential probes) against a 10s SDK timeout, so every turn paid the full timeout before the agent could read the message. Other agents (claude-code/codex/cursor/hermes/pi) don't do this — they intercept the agent'sGreptool and let the agent decide when to search.What changed
plugins.allowpatch in the installersrc/cli/install-openclaw.tsnow calls the sameensureHivemindAllowlistedhelper that/hivemind_setupusesensurePluginAllowlistedsemantics (ext/openclaw/src/config/plugins-allowlist.ts:7): only patchesplugins.allowwhen it's an explicit non-empty array. Absent / empty → left alone (default-allow stays default-allow; we don't flip the user into restrictive mode and break their other plugins)tools.alsoAllowBlocking auto-recall removed
openclaw/src/index.ts:before_agent_startno longer runssearchDeeplakeTableshivemind_search/hivemind_read/hivemind_indextools (already registered), with theSKILL.mdbody in the system prompt directing the agent to use them<recalled-memories>capture bug (old recall context was getting captured as part of the user's prompt and stored) is gone tooextractKeywords+RECALL_STOPWORDSremoved as dead codeTests
openclaw/tests/setup-command.test.ts— +9 cases forplugins.allowpatching: idempotency, default-allow semantics, both-arrays patched,detectAllowlistMissingextensionopenclaw/tests/install-openclaw.test.ts(new, 12 cases) — drives the real installer against tmpdir fixtures: file copy, both patches, idempotency, default-allow preservation, malformed config tolerance, restart hint, no-duplicateopenclaw/tests/auto-recall.test.ts— rewritten: asserts no Deeplake call on a normal turn, no<recalled-memories>injection, tools still registered, login-nudge path preservedAll 2214 hivemind tests pass.
Real-world E2E on a live openclaw gateway (2026.4.21)
Hivemind plugin registeredin gateway logallowlist already covers hivemind, no extra backupAuto-recall failedlog lines after the restart with the new bundle (previous bundle emitted them on every turn that hit Deeplake slowness)hivemind_search/hivemind_read/hivemind_indextools confirmed registered inbefore_prompt_buildCaveats
Auto-capture failed: Query timeout after 10000mslines appear in the gateway log.agent_endruns after the agent responds, so this doesn't block the user, but it does drop some turns from memory until Deeplake'ssessions-table query latency improves. Separate issue, not addressed here."hivemind"toplugins.allowwas needed even though tools were allowlisted) can't be reproduced on openclaw 2026.4.21 — that version has an "auto-promote plugins when their tools are inalsoAllow" feature that masks the gating. Older openclaw versions without that feature are the broken-state case the installer'splugins.allowpatch is for.Confidence
Confidence: high, because the unit tests cover every edge case of the new config-patching logic, the local fixture E2E drove the real installer end-to-end against multiple fixture shapes, and the live gateway confirms the bundle loads + the recall hook no longer fires.Untested: openclaw versions older than 2026.4.21(no host available; the installer's plugins.allow patch is the right fix for them but unverified end-to-end on those versions).Test plan
openclaw/src/setup-config.tssemantics (esp.isPluginsAllowMissingHivemindonly-patch-explicit-arrays rule)openclaw/tests/install-openclaw.test.tsand confirms test coverage matches the design intent in the descriptionSummary by CodeRabbit
New Features
Changes