fix(claude-code): --with-hooks for MCP-standalone users (closes #508)#581
Open
rohitg00 wants to merge 2 commits into
Open
fix(claude-code): --with-hooks for MCP-standalone users (closes #508)#581rohitg00 wants to merge 2 commits into
rohitg00 wants to merge 2 commits into
Conversation
Codex's plugin CLI (codex-rs/cli/src/plugin_cmd.rs) exposes: codex plugin add <PLUGIN>[@<MARKETPLACE>] codex plugin list codex plugin remove <PLUGIN> codex plugin marketplace <add|upgrade|remove> It does NOT have a `codex plugin install` subcommand. Users following the README would hit: error: unrecognized subcommand 'install' Fix two callsites that documented the wrong command: - README.md: top-level Codex install snippet + agent matrix row - src/cli/connect/codex.ts: final info hint after `agentmemory connect codex` CHANGELOG history is left alone since those entries reference what the README said at the time and rewriting history is not the right move. Slash-command references like `/plugin install agentmemory` inside Claude Code are unrelated and remain — that's Claude Code's slash API, not the codex CLI.
…standalone users
When agentmemory is wired into Claude Code through the MCP-standalone
path (registering `@agentmemory/mcp` in `~/.claude.json` rather than
running `/plugin install agentmemory`), Claude Code never resolves
`${CLAUDE_PLUGIN_ROOT}`. Users have to copy hook commands into
`~/.claude/settings.json` with absolute paths, and those paths embed
the agentmemory version. Every npm upgrade silently breaks all 9 hooks
— MCP keeps working, so `memory_diagnose` stays green while
auto-capture is dead.
Adds the same `--with-hooks` flag the codex adapter already has:
agentmemory connect claude-code --with-hooks
Resolves `${CLAUDE_PLUGIN_ROOT}` to the absolute bundled plugin/ path
at install time and merges the entries from `plugin/hooks/hooks.json`
into `~/.claude/settings.json`'s top-level `hooks` field. Idempotent
re-install via the `<pluginRoot>/scripts/` substring (mirrors the
codex-hooks pattern from #564). Unrelated user hooks survive.
Reuses `findPluginRoot` + `buildMergedHooks` from codex-hooks.ts;
`buildMergedHooks` now takes an explicit manifest filename
(`hooks.codex.json` for codex, `hooks.json` for claude-code) so the
Claude-only events (`SessionEnd`, `SubagentStop`, `Notification`,
`TaskCompleted`, `PostToolUseFailure`) come through.
Also makes the adapter run the hooks branch even when MCP is already
wired — hooks and MCP are independent surfaces.
README: new "Claude Code without the plugin install" subsection
documents the trade-off + points at the recommended `/plugin install`
path first.
Tests (1086) + build pass; 5 new unit tests cover Claude-event
presence, path rewrite, user-hook preservation, idempotent re-install.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR addresses version-lock issues in Claude Code when using MCP standalone (without the plugin loader) by adding a ChangesClaude Code hook installation
Possibly related PRs
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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}
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)
src/cli/connect/claude-code.ts (1)
74-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--with-hooksis skipped on fresh--dry-runinstalls.Line 74 returns before running hook preview, so
agentmemory connect claude-code --dry-run --with-hooksdoes not show hook changes unless MCP is already wired.Suggested fix
if (opts.dryRun) { p.log.info( `[dry-run] Would ${alreadyHas ? "overwrite" : "add"} mcpServers.agentmemory in ${CLAUDE_JSON}`, ); + if (opts.withHooks) { + const hookResult = installClaudeHooks(opts); + if (hookResult.kind === "skipped") { + p.log.warn(`Claude Code hooks fallback skipped: ${hookResult.reason}.`); + } + } return { kind: "installed", mutatedPath: CLAUDE_JSON }; }🤖 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 `@src/cli/connect/claude-code.ts` around lines 74 - 79, The dry-run branch returns early (when opts.dryRun) and skips the hook preview, so running `agentmemory connect claude-code --dry-run --with-hooks` won’t show hook changes; modify the block around opts.dryRun / alreadyHas / CLAUDE_JSON to not return before running the hook-preview logic: after logging the `[dry-run] Would ...` message, if opts.withHooks (the CLI flag for --with-hooks) is set, invoke the same hook preview function/path used elsewhere in this file (the code that computes and displays hook changes) and then return the { kind: "installed", mutatedPath: CLAUDE_JSON } result; alternatively move the hook-preview invocation before the return so dry-run shows hooks for fresh installs as well.
🧹 Nitpick comments (1)
test/claude-code-with-hooks.test.ts (1)
34-37: ⚡ Quick winStrengthen Claude-only event assertion to prevent partial regressions.
Using
some(...)allows this test to pass when only one Claude-only event exists; assert each expected event explicitly.Suggested refactor
- expect( - claudeOnly.some((e) => events.includes(e)), - `hooks.json should include at least one Claude-only event (${claudeOnly.join(", ")})`, - ).toBe(true); + for (const event of claudeOnly) { + expect(events).toContain(event); + }🤖 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 `@test/claude-code-with-hooks.test.ts` around lines 34 - 37, The test currently uses claudeOnly.some(...) which only verifies that at least one Claude-only event is present; change it to assert all expected Claude-only events are included by using claudeOnly.every(e => events.includes(e)) and update the failure message to reflect that every Claude-only event must appear (reference symbols: claudeOnly, events, the expect(...).toBe(true) assertion). Ensure the assertion logic is replaced so the test fails if any single expected Claude-only event is missing.
🤖 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/cli/connect/claude-code.ts`:
- Around line 131-147: installClaudeHooks currently only catches errors from
findPluginRoot; any failures while reading/parsing CLAUDE_SETTINGS, building
merged hooks via buildMergedHooks, or writing settings will throw and abort the
connect flow—wrap the risky operations (readJsonSafe(CLAUDE_SETTINGS),
buildMergedHooks(existingHooks, pluginRoot, "hooks.json"), and the settings
write path) in try/catch blocks and on error return a ConnectResult with kind:
"skipped" and reason set to the error message (preserve existing pattern err
instanceof Error ? err.message : String(err)); apply the same defensive handling
to the analogous code around the 164-166 region so manifest merge/write failures
become non-fatal warnings instead of exceptions.
---
Outside diff comments:
In `@src/cli/connect/claude-code.ts`:
- Around line 74-79: The dry-run branch returns early (when opts.dryRun) and
skips the hook preview, so running `agentmemory connect claude-code --dry-run
--with-hooks` won’t show hook changes; modify the block around opts.dryRun /
alreadyHas / CLAUDE_JSON to not return before running the hook-preview logic:
after logging the `[dry-run] Would ...` message, if opts.withHooks (the CLI flag
for --with-hooks) is set, invoke the same hook preview function/path used
elsewhere in this file (the code that computes and displays hook changes) and
then return the { kind: "installed", mutatedPath: CLAUDE_JSON } result;
alternatively move the hook-preview invocation before the return so dry-run
shows hooks for fresh installs as well.
---
Nitpick comments:
In `@test/claude-code-with-hooks.test.ts`:
- Around line 34-37: The test currently uses claudeOnly.some(...) which only
verifies that at least one Claude-only event is present; change it to assert all
expected Claude-only events are included by using claudeOnly.every(e =>
events.includes(e)) and update the failure message to reflect that every
Claude-only event must appear (reference symbols: claudeOnly, events, the
expect(...).toBe(true) assertion). Ensure the assertion logic is replaced so the
test fails if any single expected Claude-only event is missing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a26ee70-5963-4d8f-b662-229dcd557be2
📒 Files selected for processing (5)
README.mdsrc/cli/connect/claude-code.tssrc/cli/connect/codex-hooks.tssrc/cli/connect/codex.tstest/claude-code-with-hooks.test.ts
Comment on lines
+131
to
+147
| function installClaudeHooks(opts: ConnectOptions): ConnectResult { | ||
| let pluginRoot: string; | ||
| try { | ||
| pluginRoot = findPluginRoot(); | ||
| } catch (err) { | ||
| return { | ||
| kind: "skipped", | ||
| reason: err instanceof Error ? err.message : String(err), | ||
| }; | ||
| } | ||
|
|
||
| type ClaudeSettings = { hooks?: HookManifest["hooks"]; [key: string]: unknown }; | ||
| const existing = readJsonSafe<ClaudeSettings>(CLAUDE_SETTINGS) ?? {}; | ||
| const existingHooks: HookManifest | null = existing.hooks | ||
| ? { hooks: existing.hooks } | ||
| : null; | ||
| const merged = buildMergedHooks(existingHooks, pluginRoot, "hooks.json"); |
There was a problem hiding this comment.
Make hook fallback non-fatal by catching merge/write failures.
installClaudeHooks() only guards findPluginRoot(). If manifest read/parse or settings write fails, it throws and can abort the whole connect flow instead of returning kind: "skipped" for the warning path.
Suggested fix
function installClaudeHooks(opts: ConnectOptions): ConnectResult {
let pluginRoot: string;
try {
pluginRoot = findPluginRoot();
} catch (err) {
return {
kind: "skipped",
reason: err instanceof Error ? err.message : String(err),
};
}
- type ClaudeSettings = { hooks?: HookManifest["hooks"]; [key: string]: unknown };
- const existing = readJsonSafe<ClaudeSettings>(CLAUDE_SETTINGS) ?? {};
- const existingHooks: HookManifest | null = existing.hooks
- ? { hooks: existing.hooks }
- : null;
- const merged = buildMergedHooks(existingHooks, pluginRoot, "hooks.json");
+ try {
+ type ClaudeSettings = { hooks?: HookManifest["hooks"]; [key: string]: unknown };
+ const existing = readJsonSafe<ClaudeSettings>(CLAUDE_SETTINGS) ?? {};
+ const existingHooks: HookManifest | null = existing.hooks
+ ? { hooks: existing.hooks }
+ : null;
+ const merged = buildMergedHooks(existingHooks, pluginRoot, "hooks.json");
- if (opts.dryRun) {
- p.log.info(
- `[dry-run] Would merge agentmemory hook entries into ${CLAUDE_SETTINGS} (${Object.keys(merged.hooks).length} event(s))`,
- );
- return { kind: "installed", mutatedPath: CLAUDE_SETTINGS };
- }
+ if (opts.dryRun) {
+ p.log.info(
+ `[dry-run] Would merge agentmemory hook entries into ${CLAUDE_SETTINGS} (${Object.keys(merged.hooks).length} event(s))`,
+ );
+ return { kind: "installed", mutatedPath: CLAUDE_SETTINGS };
+ }
- let backupPath: string | undefined;
- if (existsSync(CLAUDE_SETTINGS)) {
- backupPath = backupFile(CLAUDE_SETTINGS, "claude-settings", "json");
- logBackup(backupPath);
- } else {
- mkdirSync(CLAUDE_DIR, { recursive: true });
- }
+ let backupPath: string | undefined;
+ if (existsSync(CLAUDE_SETTINGS)) {
+ backupPath = backupFile(CLAUDE_SETTINGS, "claude-settings", "json");
+ logBackup(backupPath);
+ } else {
+ mkdirSync(CLAUDE_DIR, { recursive: true });
+ }
- const next: ClaudeSettings = { ...existing, hooks: merged.hooks };
- writeJsonAtomic(CLAUDE_SETTINGS, next);
+ const next: ClaudeSettings = { ...existing, hooks: merged.hooks };
+ writeJsonAtomic(CLAUDE_SETTINGS, next);
- logInstalled("Claude Code hooks (workaround for `#508`)", CLAUDE_SETTINGS);
- p.log.info(
- "User-scope hook entries reference absolute paths under the bundled plugin/ dir. Re-run `agentmemory connect claude-code --with-hooks` after upgrading agentmemory to refresh them.",
- );
+ logInstalled("Claude Code hooks (workaround for `#508`)", CLAUDE_SETTINGS);
+ p.log.info(
+ "User-scope hook entries reference absolute paths under the bundled plugin/ dir. Re-run `agentmemory connect claude-code --with-hooks` after upgrading agentmemory to refresh them.",
+ );
- return {
- kind: "installed",
- mutatedPath: CLAUDE_SETTINGS,
- ...(backupPath !== undefined && { backupPath }),
- };
+ return {
+ kind: "installed",
+ mutatedPath: CLAUDE_SETTINGS,
+ ...(backupPath !== undefined && { backupPath }),
+ };
+ } catch (err) {
+ return {
+ kind: "skipped",
+ reason: err instanceof Error ? err.message : String(err),
+ };
+ }
}Also applies to: 164-166
🤖 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 `@src/cli/connect/claude-code.ts` around lines 131 - 147, installClaudeHooks
currently only catches errors from findPluginRoot; any failures while
reading/parsing CLAUDE_SETTINGS, building merged hooks via buildMergedHooks, or
writing settings will throw and abort the connect flow—wrap the risky operations
(readJsonSafe(CLAUDE_SETTINGS), buildMergedHooks(existingHooks, pluginRoot,
"hooks.json"), and the settings write path) in try/catch blocks and on error
return a ConnectResult with kind: "skipped" and reason set to the error message
(preserve existing pattern err instanceof Error ? err.message : String(err));
apply the same defensive handling to the analogous code around the 164-166
region so manifest merge/write failures become non-fatal warnings instead of
exceptions.
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
1 participant
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.
Summary
Closes #508.
When agentmemory is wired into Claude Code via the MCP-standalone path (registering
@agentmemory/mcpin~/.claude.jsonrather than running/plugin install agentmemory), Claude Code never resolves${CLAUDE_PLUGIN_ROOT}. To get auto-capture working, users have to copy hook commands into~/.claude/settings.jsonwith absolute paths — and those paths typically embed the agentmemory version (e.g.~/.codex/plugins/cache/agentmemory/agentmemory/0.9.18/scripts/…). Every npm upgrade silently breaks all 9 hooks while MCP keeps working, somemory_diagnosereturns all-green and the regression is invisible.@jonathanzhan1975 quantified the impact on the 0.9.18 → 0.9.20 bump:
Fix
Adds the same
--with-hooksopt-in the codex adapter already has (#564):${CLAUDE_PLUGIN_ROOT}to the absolute bundledplugin/path at install time.plugin/hooks/hooks.jsoninto~/.claude/settings.json's top-levelhooksfield.<pluginRoot>/scripts/; unrelated user hooks survive untouched.already-wiredearly return now also calls the hooks branch when--with-hooksis passed).--dry-run --with-hookspreviews the change.Mechanism
Reuses
findPluginRoot+buildMergedHooksfromcodex-hooks.ts.buildMergedHooksnow takes the manifest filename explicitly:hooks.codex.jsonfor the codex adapter (Codex's 6-event subset)hooks.jsonfor the claude-code adapter (full 12-event Claude set includingSessionEnd,SubagentStop,Notification,TaskCompleted,PostToolUseFailure)Diff
src/cli/connect/claude-code.ts—+86addsinstallClaudeHooks()+ wire-upsrc/cli/connect/codex-hooks.ts—+5/-2parametrize the manifest filenametest/claude-code-with-hooks.test.ts—+72new, 5 unit testsREADME.md—+12new "Claude Code without the plugin install" subsectionValidation
npm test→ 98/98 test files, 1086/1086 tests passnpm run build→ bundle cleannode dist/cli.mjs connect claude-code --dry-run --with-hookspreviews 12 events, runs even when MCP is already wired.Recommended user flow
The README block points at the proper
/plugin marketplace add+/plugin installpath first;--with-hooksis the explicit escape hatch for setups that can't or won't use the Claude plugin install.Summary by CodeRabbit
Documentation
New Features
Tests