Skip to content

fix(codex/agents): VFS write routing, plugin-disable, embed-deps symlinks#246

Merged
efenocchi merged 11 commits into
mainfrom
feat/session-briefs-and-channel-safety
Jun 8, 2026
Merged

fix(codex/agents): VFS write routing, plugin-disable, embed-deps symlinks#246
efenocchi merged 11 commits into
mainfrom
feat/session-briefs-and-channel-safety

Conversation

@efenocchi
Copy link
Copy Markdown
Collaborator

@efenocchi efenocchi commented Jun 8, 2026

Summary

  • VFS write routing fix (pre-tool-use): echo/printf/tee redirects targeting the memory VFS now correctly route through the VFS shell for Codex, Cursor, and Hermes
  • Codex action fix: successful VFS writes return action=guide (exit 0) instead of action=block (exit 2) — Codex reports success, not "command blocked"; tightened isWriteRedirect to only match echo|printf|tee so mixed commands cannot escape to the host shell
  • Plugin-disable support (plugin-state.ts): Codex, Cursor, and Hermes now respect hivemind disable / re-enable at runtime, matching Claude Code behavior
  • embed-deps symlinks (install-codex/cursor/hermes.ts): install scripts create a symlink into ~/.hivemind/embed-deps/node_modules so native modules resolve correctly
  • Debug log dir guard (debug.ts): wrap filesystem calls in try-catch for best-effort logging; create log dir before appendFileSync to prevent ENOENT
  • VFS unroutable fallback (pre-tool-use): unsafe commands route to deeplake-cli or show an install prompt; single-quote the -c payload to block shell expansion; guard Read fallback to preserve file_path contract
  • Skillify trigger fix (all agents): forceSessionEndTrigger now 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 first

Version 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 files
  • Codex live: hivemind install --only codex, write a file to the VFS inside a Codex session — PreToolUse intercepts, Stop hook exits 0
  • Skillify e2e: invoked session-end.js with a held wiki-worker lock — skillify.log appeared and worker mined 6 sessions
  • All 4 agents tested for lock-contention: forceSessionEndTrigger fires even when tryAcquireLock returns false

Generated with Claude Code

efenocchi added 6 commits June 8, 2026 00:08
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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 8, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds runtime gates that skip capture/session hooks when the hivemind plugin is disabled, wires installer-time symlinks from ~/.hivemind/embed-deps/node_modules into plugin installs (removing real directories first), falls back unroutable memory commands to a bundled sandbox (deeplake-shell.js), and ensures debug/daemon directories exist before writing sockets/logs.

Changes

Plugin Enablement and Dependency Wiring

Layer / File(s) Summary
Plugin enablement detection utility
src/utils/plugin-state.ts, tests/shared/plugin-state.test.ts
New isHivemindPluginEnabled() reads ~/.claude/settings.json and checks whether hivemind@hivemind is not explicitly disabled, defaulting to true if missing or unparseable. Tests cover missing files, missing keys, explicit true/false, and corrupt JSON.
Installation node_modules symlink wiring
src/cli/install-codex.ts, src/cli/install-cursor.ts, src/cli/install-hermes.ts
Codex, Cursor, and Hermes installers each conditionally symlink ~/.hivemind/embed-deps/node_modules into their plugin directories when the embed-deps location exists. Each checks whether the target is a real directory (not a symlink), removes it, then forces the symlink.
Capture hooks plugin-enabled gates
src/hooks/capture.ts, src/hooks/codex/capture.ts, src/hooks/cursor/capture.ts, src/hooks/hermes/capture.ts
Main and variant capture hooks add early-return guards that skip capture processing and session-event writes when isHivemindPluginEnabled() returns false.
Pre-tool-use sandbox fallback for unroutable commands
src/hooks/pre-tool-use.ts, src/hooks/codex/pre-tool-use.ts, tests/claude-code/pre-tool-use-branches.test.ts, tests/claude-code/pre-tool-use.test.ts
Pre-tool-use hooks now fall back to running deeplake-shell.js via spawnSync when memory commands cannot be routed through inline handlers. Sandbox execution determines whether to return guide (on success for write-redirects) or block (on failure). Tests updated and comments added to assert commands use the VFS shell bundle instead of returning retry guidance and to clarify no-config retry behavior.
Session-end hook ordering & plugin-enabled gate
src/hooks/session-end.ts, src/hooks/codex/stop.ts, src/hooks/cursor/session-end.ts, src/hooks/hermes/session-end.ts, tests/*-session-end-hook.test.ts
Session-end hooks load config/cwd early, invoke forceSessionEndTrigger before acquiring periodic/wiki-worker locks, and skip processing when the plugin is disabled. Tests mock forceSessionEndTrigger and add regression cases asserting the trigger still fires under lock contention while the wiki worker spawn is skipped.
Debug and daemon directory creation
embeddings/embed-daemon.js, src/utils/debug.ts
Daemon startup creates the socket parent directory via an aliased mkdirSync2, and debug logging ensures the hook-debug.log parent directory exists before appending. Both use recursive: true for idempotent directory creation.
CLI install-cursor tests
tests/cli/cli-install-cursor-fs.test.ts
Tests verify symlink creation, replacement of a real directory by a symlink, and absence of symlink when embed-deps is missing.

Sequence Diagram

sequenceDiagram
  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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • activeloopai/hivemind#210: Also modifies Claude Code hook entrypoints and runtime gating behavior related to capture/session hooks.
  • activeloopai/hivemind#98: Reorders and integrates Skillify trigger workflows in session-end hooks; overlaps session-end ordering changes.
  • activeloopai/hivemind#128: Alters session-end main() flow and touches nearby session-end logic that may overlap.

Suggested reviewers

  • kaghni
  • khustup2

Poem

🐰 A plugin now checks if it's truly awake,
Symlinks the deps so the installs don't break,
When memory commands wander off in the night,
The sandbox shell runner brings back the light,
Directories safe, logs tucked in tight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately summarizes the three main changes: VFS write routing fix, plugin-disable support, and embed-deps symlinks setup.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed PR description is detailed and complete, covering summary, changes, version bump decision, test plan with specific commands and verification steps.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/session-briefs-and-channel-safety

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread tests/shared/plugin-state.test.ts Fixed
Comment thread tests/shared/plugin-state.test.ts Fixed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 8, 2026

Coverage Report

Scope: files changed in this PR. Enforced threshold: 90% per metric (per file via vitest.config.ts).

Status Category Percentage Covered / Total
🟢 Lines 96.01% (🎯 90%) 1034 / 1077
🟢 Statements 94.35% (🎯 90%) 1186 / 1257
🟢 Functions 97.27% (🎯 90%) 107 / 110
🔴 Branches 85.45% (🎯 90%) 822 / 962
File Coverage — 15 files changed
File Stmts Branches Functions Lines
src/cli/install-codex.ts 🟢 96.4% 🔴 86.6% 🟢 100.0% 🟢 96.5%
src/cli/install-cursor.ts 🟢 97.1% 🔴 88.1% 🟢 100.0% 🟢 100.0%
src/cli/install-hermes.ts 🟢 92.1% 🔴 81.5% 🟢 100.0% 🟢 96.5%
src/hooks/capture.ts 🟢 95.2% 🔴 82.0% 🟢 100.0% 🟢 100.0%
src/hooks/codex/capture.ts 🟢 97.2% 🔴 83.3% 🟢 100.0% 🟢 100.0%
src/hooks/codex/pre-tool-use.ts 🟢 95.2% 🔴 80.2% 🔴 80.0% 🟢 96.2%
src/hooks/codex/stop.ts 🟢 98.6% 🟢 90.5% 🟢 100.0% 🟢 98.4%
src/hooks/cursor/capture.ts 🟢 92.0% 🔴 88.3% 🟢 100.0% 🟢 95.9%
src/hooks/cursor/session-end.ts 🟢 96.0% 🔴 85.7% 🟢 100.0% 🟢 94.4%
src/hooks/hermes/capture.ts 🔴 85.1% 🔴 82.0% 🟢 100.0% 🔴 87.0%
src/hooks/hermes/session-end.ts 🟢 96.2% 🔴 85.0% 🟢 100.0% 🟢 94.7%
src/hooks/pre-tool-use.ts 🟢 94.2% 🔴 88.2% 🟢 94.7% 🟢 93.9%
src/hooks/session-end.ts 🟢 93.5% 🔴 88.5% 🟢 100.0% 🟢 100.0%
src/utils/debug.ts 🟢 100.0% 🟢 100.0% 🟢 100.0% 🟢 100.0%
src/utils/plugin-state.ts 🟢 100.0% 🟢 100.0% 🟢 100.0% 🟢 100.0%

Generated for commit 849a5e9.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
tests/claude-code/pre-tool-use-branches.test.ts (1)

554-558: ⚡ Quick win

Assert 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 exact node "<bundle>" -c "<escaped rewritten command>" shape, pin the full command or at least the node ... -c prefix 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

📥 Commits

Reviewing files that changed from the base of the PR and between 79657e6 and 18f0398.

📒 Files selected for processing (16)
  • embeddings/embed-daemon.js
  • src/cli/install-codex.ts
  • src/cli/install-cursor.ts
  • src/cli/install-hermes.ts
  • src/hooks/capture.ts
  • src/hooks/codex/capture.ts
  • src/hooks/codex/pre-tool-use.ts
  • src/hooks/cursor/capture.ts
  • src/hooks/hermes/capture.ts
  • src/hooks/pre-tool-use.ts
  • src/hooks/session-end.ts
  • src/utils/debug.ts
  • src/utils/plugin-state.ts
  • tests/claude-code/pre-tool-use-branches.test.ts
  • tests/claude-code/pre-tool-use.test.ts
  • tests/shared/plugin-state.test.ts

Comment thread src/hooks/codex/pre-tool-use.ts
Comment thread src/hooks/pre-tool-use.ts
Comment thread src/hooks/pre-tool-use.ts
Comment thread src/utils/debug.ts
Comment thread tests/shared/plugin-state.test.ts Outdated
- 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
Comment thread src/utils/debug.ts
appendFileSync(LOG, `${new Date().toISOString()} [${tag}] ${msg}\n`);
try {
mkdirSync(dirname(LOG), { recursive: true });
appendFileSync(LOG, `${new Date().toISOString()} [${tag}] ${msg}\n`);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

efenocchi added 4 commits June 8, 2026 03:40
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.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
tests/cli/cli-install-cursor-fs.test.ts (2)

120-130: ⚡ Quick win

Consider verifying the symlink target.

The test confirms a symlink is created but doesn't verify it points to embedDepsNm. Add readlinkSync(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 win

Consider 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2476cfc and ee345fe.

📒 Files selected for processing (12)
  • src/hooks/codex/capture.ts
  • src/hooks/codex/pre-tool-use.ts
  • src/hooks/codex/stop.ts
  • src/hooks/cursor/session-end.ts
  • src/hooks/hermes/capture.ts
  • src/hooks/hermes/session-end.ts
  • src/hooks/session-end.ts
  • tests/claude-code/session-end-hook.test.ts
  • tests/cli/cli-install-cursor-fs.test.ts
  • tests/codex/codex-stop-hook.test.ts
  • tests/cursor/cursor-session-end-hook.test.ts
  • tests/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({
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this new method cover spawnHermesWikiWorker? @coderabbitai

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. 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.
  2. tryAcquireLock (lines 46–49) — guards the wiki worker from running concurrently.
  3. spawnHermesWikiWorker (lines 51–60) — still present in its own try/catch block; spawns the hermes -z wiki 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]
Loading

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.

@efenocchi efenocchi merged commit 0e93c13 into main Jun 8, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants