fix(codex/agents): VFS write routing, plugin-disable, embed-deps symlinks#246
Conversation
appendFileSync fails when ~/.deeplake/ does not exist (e.g. tests that set a temp HOME). Add mkdirSync with recursive:true so the dir is created on first debug write, making the logger self-contained.
Commands that pass isSafe() but have no inline handler (echo redirects, jq pipes, find with unsupported flags) previously returned RETRY REQUIRED. They now fall through to the VFS shell bundle which handles them in a sandboxed Node.js interpreter against the SQL backend — no host filesystem access occurs. Fixes: echo/printf writes, pipe chains, and non-standard find patterns all working end-to-end against the Deeplake SQL memory table.
Hooks are loaded at SessionStart and stay active for the session lifetime even after `claude plugin disable hivemind`. Each capture and session-end invocation now reads enabledPlugins from ~/.claude/settings.json at runtime and exits early if the plugin is disabled — no capture, no wiki worker. Fails open: a missing or corrupt settings.json is treated as enabled so a bad file does not silently drop all captures.
VFS write (pre-tool-use):
- Codex: unroutable isSafe commands now run via spawnSync against the VFS
shell bundle and return {action:"block", output} — prevents host shell
access while delivering the write/pipe result to the agent. Falls back
to guidance if the shell exits non-zero (e.g. bundle missing in tests).
Plugin disable (#94):
- Codex/Cursor/Hermes capture hooks now read ~/.claude/settings.json at
runtime and exit early when enabledPlugins[hivemind@hivemind]=false,
matching the Claude Code fix from the previous commit.
Verified: 4241 unit tests pass; end-to-end plugin-disable confirmed via
log output on all four agents.
graph-on-stop.js uses tree-sitter as an external native module — esbuild cannot bundle it. The plugin dir's node_modules must resolve to ~/.hivemind/embed-deps/node_modules (which contains tree-sitter after hivemind embeddings install). installCodex/Cursor/Hermes now replace any empty placeholder dir at <pluginDir>/node_modules with a symlink to embed-deps, matching what ensurePluginNodeModulesLink does at runtime for Claude Code. Without this, graph-on-stop exits 1 with ERR_MODULE_NOT_FOUND on every Codex/ Cursor/Hermes session end.
…ts success When the VFS shell executes a write redirect (echo/printf/tee … > file) and exits 0, return action="guide" instead of "block". "block" exits 2 which makes Codex report "command blocked" even when the SQL write succeeded — confusing the agent. "guide" exits 0 so Codex treats the command as successful and shows the output to the agent. Non-redirect commands (pipes, finds, reads that fall through) still use "block" to prevent unsafe host-shell execution.
📝 WalkthroughWalkthroughThis PR adds runtime gates that skip capture/session hooks when the hivemind plugin is disabled, wires installer-time symlinks from ChangesPlugin Enablement and Dependency Wiring
Sequence DiagramsequenceDiagram
participant User as CLI User
participant Installer as Installer (Codex/Cursor/Hermes)
participant EmbedDeps as ~/.hivemind/embed-deps
participant PluginNM as Plugin node_modules
participant PreTool as Pre-tool-use Hook
participant Handler as Inline Handler
participant Sandbox as deeplake-shell.js
participant Capture as Capture Hook
participant State as Plugin State Check
User ->> Installer: Run install command
Installer ->> EmbedDeps: Check if embed-deps/node_modules exists
alt embed-deps exists
Installer ->> PluginNM: Remove if real directory
Installer ->> PluginNM: Create symlink from embed-deps
end
User ->> PreTool: Execute memory command
PreTool ->> Handler: Try inline route
alt handler can't route
PreTool ->> Sandbox: Fall back to sandbox shell
Sandbox -->> PreTool: Return output/exit code
PreTool ->> User: guide or block decision
end
User ->> Capture: Session event
Capture ->> State: Check isHivemindPluginEnabled
alt plugin disabled
State -->> Capture: false
Capture ->> User: Skip capture, return early
else plugin enabled
State -->> Capture: true
Capture ->> User: Process event normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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 — 15 files changed
Generated for commit 849a5e9. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/claude-code/pre-tool-use-branches.test.ts (1)
554-558: ⚡ Quick winAssert the full shell rewrite, not just the bundle name.
These checks still pass if the hook returns an unrelated command that merely mentions
deeplake-shell.js. Since this branch's safety depends on the exactnode "<bundle>" -c "<escaped rewritten command>"shape, pin the full command or at least thenode ... -cprefix plus the rewritten payload for each case.As per coding guidelines, tests/** should prefer asserting on specific values over generic substrings.
Also applies to: 605-607, 640-641
🤖 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 `@tests/claude-code/pre-tool-use-branches.test.ts` around lines 554 - 558, The test currently checks only that d?.command contains "deeplake-shell.js" and not "RETRY REQUIRED"; update it to assert the full rewritten shell invocation shape used by the branch (e.g., the exact `node "<bundle>" -c "<escaped rewritten command>"` payload) by replacing the loose substring assertions on d?.command with a precise equality or a check that it startsWith `node "` and contains `-c "` plus the expected escaped command string; apply the same change for the other similar assertions referenced (around lines 605-607 and 640-641) and keep the existing expect(findVirtualPathsFn).not.toHaveBeenCalled() check.Source: Coding guidelines
🤖 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 `@src/hooks/codex/pre-tool-use.ts`:
- Around line 363-381: The current fallback uses isWriteRedirect (/\s>>?\s/) to
return action "guide" which can cause mixed host reads/writes to run on the
host; instead always return action "block" for unroutable memory commands so the
host shell is never invoked. In the spawnSync handling around rewritten and
isWriteRedirect, remove or ignore the isWriteRedirect branch and always set
action to "block" (keep returning the sandbox output and rewrittenCommand),
ensuring no path returns "guide" unless you implement a stricter, proof-based
check that verifies the entire command's inputs/outputs are only inside the VFS
memory paths before changing behavior.
In `@src/hooks/pre-tool-use.ts`:
- Around line 573-584: The fallback currently always returns a command-shaped
buildAllowDecision (using shellBundle/shellCmd), which breaks Read requests
because main() expects updatedInput.file_path; modify the fall-through so that
if the tool request type is "Read" you return a file-shaped decision (populate
updatedInput.file_path with a cached file path or return a deny-style fallback
decision) instead of a command-shaped buildAllowDecision, otherwise keep the
existing command-shaped behavior; update the logic around
shellBundle/shellCmd/buildAllowDecision to branch on the request kind (Read vs
others) so main() receives file_path for Read and command/description for
shell-style fallbacks.
- Around line 578-583: The current fallback builds node "${shellBundle}" -c
"${escaped}" where escaped only handles backslashes and double-quotes, allowing
host-shell expansions and also returns an allow decision without a file_path;
change this to (1) compose the -c payload as a single argv-safe single-quoted
shell argument by replacing each single-quote in shellCmd with the standard '\''
sequence (i.e. close single quote, escape a single quote, reopen) so the entire
payload is wrapped in single-quotes and immune to $() and backticks, using the
existing shellBundle and shellCmd variables; and (2) if input.tool_name ===
"Read" short-circuit this fallback and return the same deny/retry
(file_path-shaped) decision used elsewhere for Read failures instead of calling
buildAllowDecision, so the Read contract is preserved.
In `@src/utils/debug.ts`:
- Around line 22-26: The log function (export function log) should be made
best-effort by wrapping the filesystem operations (mkdirSync(dirname(LOG), ...)
and appendFileSync(LOG, ...)) in a try-catch so any filesystem errors
(permission denied, ENOSPC, etc.) are swallowed or handled without throwing;
keep the outer isDebug() check, and on error optionally emit a non-throwing
notification (e.g., console.error or process.stderr.write) but do not
rethrow—update the log function to catch exceptions around the mkdirSync and
appendFileSync calls and return silently on failure.
In `@tests/shared/plugin-state.test.ts`:
- Around line 21-22: Replace the manual temp path creation using Date.now() and
mkdirSync with a secure mkdtempSync-based temp dir: where the code sets tmpDir
(and any other spots creating a temp path using join(tmpdir(),
`plugin-state-test-${Date.now()}`)), call fs.mkdtempSync(path.join(os.tmpdir(),
'plugin-state-test-')) to create a unique temp directory and remove the
subsequent mkdirSync; update any teardown logic that references tmpDir to use
the new mkdtempSync-created path.
---
Nitpick comments:
In `@tests/claude-code/pre-tool-use-branches.test.ts`:
- Around line 554-558: The test currently checks only that d?.command contains
"deeplake-shell.js" and not "RETRY REQUIRED"; update it to assert the full
rewritten shell invocation shape used by the branch (e.g., the exact `node
"<bundle>" -c "<escaped rewritten command>"` payload) by replacing the loose
substring assertions on d?.command with a precise equality or a check that it
startsWith `node "` and contains `-c "` plus the expected escaped command
string; apply the same change for the other similar assertions referenced
(around lines 605-607 and 640-641) and keep the existing
expect(findVirtualPathsFn).not.toHaveBeenCalled() check.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72024bca-7bdb-4b64-bbdd-113974e8703d
📒 Files selected for processing (16)
embeddings/embed-daemon.jssrc/cli/install-codex.tssrc/cli/install-cursor.tssrc/cli/install-hermes.tssrc/hooks/capture.tssrc/hooks/codex/capture.tssrc/hooks/codex/pre-tool-use.tssrc/hooks/cursor/capture.tssrc/hooks/hermes/capture.tssrc/hooks/pre-tool-use.tssrc/hooks/session-end.tssrc/utils/debug.tssrc/utils/plugin-state.tstests/claude-code/pre-tool-use-branches.test.tstests/claude-code/pre-tool-use.test.tstests/shared/plugin-state.test.ts
- codex/pre-tool-use: tighten isWriteRedirect to echo|printf|tee only so mixed commands like `sort /etc/passwd > /vfs/out` never get guide action (guide lets Codex re-run the original on the host) - pre-tool-use: single-quote the VFS shell -c argument to prevent $(), backtick, and variable expansion before deeplake-shell.js receives the payload; guard Read fallback to return deny+guidance instead of a command-shaped allow that violates the file_path contract - debug.ts: wrap filesystem calls in try-catch for best-effort logging - tests/shared/plugin-state.test.ts: use mkdtempSync to avoid predictable temp paths and CodeQL insecure-temp-file findings
| appendFileSync(LOG, `${new Date().toISOString()} [${tag}] ${msg}\n`); | ||
| try { | ||
| mkdirSync(dirname(LOG), { recursive: true }); | ||
| appendFileSync(LOG, `${new Date().toISOString()} [${tag}] ${msg}\n`); |
There was a problem hiding this comment.
False positive. Three constraints make this safe: (1) LOG is a hardcoded path (~/.deeplake/hook-debug.log) so no path-traversal is possible, (2) the log is write-only — the content is never parsed or executed, (3) the log() function is guarded by if (!isDebug()) return so it only runs when HIVEMIND_DEBUG=1 is explicitly set by the operator. The network data flowing into the log is intentional diagnostic output (SQL query text, error messages) with no injection surface.
forceSessionEndTrigger was placed after the wiki-worker tryAcquireLock check. When the Periodic trigger from capture.ts had already acquired that lock, session-end.ts returned early and the skillify worker was never spawned — skills were never generated for authenticated users. Move forceSessionEndTrigger before the wiki-worker lock check: the skillify trigger has its own per-project lock so it does not need the wiki-worker to be free. Also fall back to process.cwd() when input.cwd is absent so the project key can always be derived.
…mes/Codex Same bug as Claude Code (fixed in b63105a): forceSessionEndTrigger was placed after the wiki-worker tryAcquireLock check in all three agents. When the Periodic trigger already held the lock, session-end returned early and skill mining was silently skipped. Move forceSessionEndTrigger before the lock check in cursor/session-end, hermes/session-end, and codex/stop. Also add process.cwd() fallback for Codex which used `input.cwd ?? ""`. Add lock-contention regression tests to all four agents asserting that forceSessionEndTrigger fires even when tryAcquireLock returns false.
…hreshold Three new cases for the lstatSync/symlinkForce block in installCursor: - embed-deps exists, pluginNm absent (lstatSync throws → catch, then symlink) - embed-deps exists, pluginNm is a real directory (rmSync + symlink) - embed-deps absent (skip the block entirely) Raises branch coverage from 76% to ≥80% to clear the CI threshold.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/cli/cli-install-cursor-fs.test.ts (2)
120-130: ⚡ Quick winConsider verifying the symlink target.
The test confirms a symlink is created but doesn't verify it points to
embedDepsNm. AddreadlinkSync(pluginNm)to assert the target matches the expected path.🔗 Proposed enhancement to verify symlink target
const { installCursor } = await importInstaller(); installCursor(); const pluginNm = join(tmpHome, ".cursor", "hivemind", "node_modules"); - const { lstatSync } = await import("node:fs"); + const { lstatSync, readlinkSync } = await import("node:fs"); expect(lstatSync(pluginNm).isSymbolicLink()).toBe(true); + expect(readlinkSync(pluginNm)).toBe(embedDepsNm); });🤖 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 `@tests/cli/cli-install-cursor-fs.test.ts` around lines 120 - 130, The test verifies a symlink exists but doesn't assert its target; update the test in the "creates embed-deps symlink..." spec to also read the symlink target and compare it to the expected embedDepsNm path (use readlinkSync on pluginNm and assert it equals embedDepsNm). Locate the variables embedDepsNm and pluginNm in the test and add a readlinkSync(pluginNm) assertion after the existing isSymbolicLink() check to ensure the symlink points to the intended embed-deps node_modules directory.
132-144: ⚡ Quick winConsider verifying the symlink target.
Same as the previous test: confirm the symlink points to the correct location to catch potential misconfigurations.
🔗 Proposed enhancement to verify symlink target
const { installCursor } = await importInstaller(); installCursor(); - const { lstatSync } = await import("node:fs"); + const { lstatSync, readlinkSync } = await import("node:fs"); expect(lstatSync(pluginNm).isSymbolicLink()).toBe(true); + expect(readlinkSync(pluginNm)).toBe(embedDepsNm); });🤖 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 `@tests/cli/cli-install-cursor-fs.test.ts` around lines 132 - 144, The test "replaces an existing real directory at pluginNm with a symlink when embed-deps present" currently only asserts that pluginNm is a symlink; update the test to also verify the symlink target points to the expected embedDepsNm target. After calling installCursor(), resolve or read the symlink target for pluginNm (using fs.readlink or path.resolve with fs.readlinkSync) and assert it equals the expected path (embedDepsNm or the exact node_modules path constructed as embedDepsNm), referencing the existing variables pluginNm, embedDepsNm and the installCursor helper so the assertion catches incorrect link targets.
🤖 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.
Nitpick comments:
In `@tests/cli/cli-install-cursor-fs.test.ts`:
- Around line 120-130: The test verifies a symlink exists but doesn't assert its
target; update the test in the "creates embed-deps symlink..." spec to also read
the symlink target and compare it to the expected embedDepsNm path (use
readlinkSync on pluginNm and assert it equals embedDepsNm). Locate the variables
embedDepsNm and pluginNm in the test and add a readlinkSync(pluginNm) assertion
after the existing isSymbolicLink() check to ensure the symlink points to the
intended embed-deps node_modules directory.
- Around line 132-144: The test "replaces an existing real directory at pluginNm
with a symlink when embed-deps present" currently only asserts that pluginNm is
a symlink; update the test to also verify the symlink target points to the
expected embedDepsNm target. After calling installCursor(), resolve or read the
symlink target for pluginNm (using fs.readlink or path.resolve with
fs.readlinkSync) and assert it equals the expected path (embedDepsNm or the
exact node_modules path constructed as embedDepsNm), referencing the existing
variables pluginNm, embedDepsNm and the installCursor helper so the assertion
catches incorrect link targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9597f9a-4f62-4916-8ed9-d12cf502189e
📒 Files selected for processing (12)
src/hooks/codex/capture.tssrc/hooks/codex/pre-tool-use.tssrc/hooks/codex/stop.tssrc/hooks/cursor/session-end.tssrc/hooks/hermes/capture.tssrc/hooks/hermes/session-end.tssrc/hooks/session-end.tstests/claude-code/session-end-hook.test.tstests/cli/cli-install-cursor-fs.test.tstests/codex/codex-stop-hook.test.tstests/cursor/cursor-session-end-hook.test.tstests/hermes/hermes-session-end-hook.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/hooks/hermes/capture.ts
- src/hooks/codex/capture.ts
- src/hooks/codex/pre-tool-use.ts
| // skill mining. | ||
| try { | ||
| spawnHermesWikiWorker({ | ||
| forceSessionEndTrigger({ |
There was a problem hiding this comment.
Does this new method cover spawnHermesWikiWorker? @coderabbitai
There was a problem hiding this comment.
@kaghni No, forceSessionEndTrigger does not replace spawnHermesWikiWorker — they serve independent purposes and both are still called.
What the diff shows is only the reordering of these two operations. In the full file:
forceSessionEndTrigger(lines 37–44) — fires the Skillify/skill-mining trigger. This is now called before the wiki-worker lock check so that lock contention doesn't suppress skill mining.tryAcquireLock(lines 46–49) — guards the wiki worker from running concurrently.spawnHermesWikiWorker(lines 51–60) — still present in its own try/catch block; spawns thehermes -zwiki worker to write the AI session summary into the memory table.
Here's the complete flow for clarity:
flowchart TD
A[main] --> B[forceSessionEndTrigger\nSkillify trigger]
B --> C{tryAcquireLock?}
C -- "lock held" --> D[skip wiki worker, return]
C -- "lock acquired" --> E[spawnHermesWikiWorker\nwiki summary]
The key design intent (per the comment in the code) is that a periodic trigger already holding the lock should not suppress skill mining — hence forceSessionEndTrigger runs unconditionally before the lock check, while spawnHermesWikiWorker remains lock-guarded as before.
🧠 Learnings used
Learnt from: efenocchi
Repo: activeloopai/hivemind PR: 98
File: codex/bundle/stop.js:1402-1408
Timestamp: 2026-05-06T23:01:21.197Z
Learning: In `codex/bundle/stop.js` (and `src/hooks/codex/stop.ts`), `forceSessionEndTrigger` is called unconditionally on every Codex `Stop` event by deliberate design. Codex has no separate `session_end` hook — `Stop` fires on every assistant turn and is the only available signal for session completion. Using `tryStopCounterTrigger` instead would silently break skill mining on short sessions (counter never reaches threshold, session closes, worker never fires). The per-projectKey worker lock (`tryAcquireWorkerLock` with a 10-min stale TTL in `src/skilify/state.ts`) makes the over-triggering cheap: subsequent Stops within the same session find nothing past the watermark and exit in <1s. This matches the pattern used by Cursor, Hermes, and Pi agents.
Learnt from: kaghni
Repo: activeloopai/hivemind PR: 97
File: src/hooks/shared/autoupdate.ts:122-146
Timestamp: 2026-05-07T08:28:25.207Z
Learning: In `src/hooks/shared/autoupdate.ts`, the `autoUpdate` function intentionally does NOT include a machine-wide lock/cooldown. An initial draft had a flock-style lock at `~/.deeplake/.autoupdate.lock`, but after a latency-fix refactor to fire-and-forget detached spawn (commit 601aa5c), the lock is released immediately after dispatching the child — defeating the purpose. The proper home for the lock is `src/cli/update.ts:runUpdate()` so the long-running `hivemind update` process owns the lock for its full lifetime. `npm install -g` has its own internal locking that prevents corruption in the meantime, so concurrent invocations are either both no-ops (already at latest) or one wins idempotently. This is tracked as a separate follow-up.
Learnt from: efenocchi
Repo: activeloopai/hivemind PR: 183
File: openclaw/src/index.ts:1280-1289
Timestamp: 2026-05-19T00:40:53.583Z
Learning: A future improvement to switch the entire `sessions.message_embedding` capture path to content-only embeddings (better recall, less token noise) requires updating all five agents (claude-code, codex, cursor, hermes, openclaw) and re-embedding existing rows — it is explicitly out of scope for piecemeal changes and should be a dedicated PR.
Learnt from: efenocchi
Repo: activeloopai/hivemind PR: 183
File: openclaw/src/index.ts:1280-1289
Timestamp: 2026-05-19T00:40:45.681Z
Learning: When populating the database column `sessions.message_embedding` in the session capture hooks, embed the full JSON envelope (the entire `line` object containing `id`, `type`, `session_id`, `content`, and `timestamp`) rather than embedding only the content-only `text`. Embedding content-only text for just one agent can break cross-agent semantic comparability/ranking for agents that have different embedding distributions. Separately, in the wiki-worker path, `memory.summary_embedding` intentionally uses content-only `text` because that column’s semantics differ—do not apply the session-enveloping rule there.
Summary
pre-tool-use): echo/printf/tee redirects targeting the memory VFS now correctly route through the VFS shell for Codex, Cursor, and Hermesaction=guide(exit 0) instead ofaction=block(exit 2) — Codex reports success, not "command blocked"; tightenedisWriteRedirectto only matchecho|printf|teeso mixed commands cannot escape to the host shellplugin-state.ts): Codex, Cursor, and Hermes now respecthivemind disable/ re-enable at runtime, matching Claude Code behaviorinstall-codex/cursor/hermes.ts): install scripts create a symlink into~/.hivemind/embed-deps/node_modulesso native modules resolve correctlydebug.ts): wrap filesystem calls in try-catch for best-effort logging; create log dir beforeappendFileSyncto prevent ENOENTpre-tool-use): unsafe commands route to deeplake-cli or show an install prompt; single-quote the-cpayload to block shell expansion; guard Read fallback to preservefile_pathcontractforceSessionEndTriggernow fires before the wiki-worker lock check in Claude Code, Codex, Cursor, and Hermes — skill mining was silently suppressed whenever the Periodic trigger held the lock firstVersion Bump
Patch — all changes are bug fixes and defensive hardening with no new public API or breaking changes.
Test plan
npm test— 4277 tests, 215 files, all green; coverage ≥90% on changed fileshivemind install --only codex, write a file to the VFS inside a Codex session — PreToolUse intercepts, Stop hook exits 0session-end.jswith a held wiki-worker lock —skillify.logappeared and worker mined 6 sessionsforceSessionEndTriggerfires even whentryAcquireLockreturns falseGenerated with Claude Code