diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 7534d69..1529309 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1121,17 +1121,27 @@ flowchart TD ### Plugin installs declare the same hooks instead When the dashboard is installed as the `ccam` Claude Code plugin, the eight hook -entries come from the inline `hooks` block in `.claude-plugin/plugin.json` -(each running `${CLAUDE_PLUGIN_ROOT}/scripts/hook-handler.js`), and -`install-hooks.js` is not used at all. Both at once would POST every event twice -— events carry no id, so ingest cannot deduplicate them, and every token and -cost figure would double. Two guards keep that from happening silently: +entries are declared inline in `.claude-plugin/plugin.json` (each running +`${CLAUDE_PLUGIN_ROOT}/scripts/hook-handler.js`), and `install-hooks.js` is not +used at all. `claude plugin install` itself materializes these into +`~/.claude/settings.json` with `${CLAUDE_PLUGIN_ROOT}` already resolved to the +plugin's cache path — confirmed by installing the plugin for real and +inspecting the file, not just reading the docs. Both a checkout install and a +plugin install writing hooks at once would POST every event twice — events +carry no id, so ingest cannot deduplicate them, and every token and cost figure +would double. Two guards keep that from happening silently: -- `scripts/plugin-bootstrap.js` strips `hook-handler.js` entries out of - `~/.claude/settings.json` on session start (backing the file up as - `settings.json.ccam-bak` first) and logs what it removed. +- `scripts/plugin-bootstrap.js` strips checkout-style `hook-handler.js` entries + out of `~/.claude/settings.json` on session start (backing the file up as + `settings.json.ccam-bak` first) and logs what it removed. Because the + plugin's own entries ALSO contain `hook-handler.js` (just resolved to a cache + path instead of a raw filesystem one), a plain substring match cannot tell + them apart — `isCheckoutHookEntry()` only treats an entry as removable when + its command does NOT resolve under `~/.claude/plugins/cache/`, so the + plugin's own legitimate hooks are never touched. - `install-hooks.js` warns when the plugin runtime state exists, and - `/ccam-doctor` reports any surviving duplicates as a `FAIL`. + `/ccam-doctor` reports any surviving checkout-style duplicates as a `FAIL` + using the same `isCheckoutHookEntry()` predicate. `scripts/plugin-bootstrap.js` also owns the rest of the plugin's runtime: the Node >= 22.5 gate (`node:sqlite`), an atomic `mkdir` lock, the dependency diff --git a/docs/PLUGINS.md b/docs/PLUGINS.md index 8d7fb38..5b5e26c 100644 --- a/docs/PLUGINS.md +++ b/docs/PLUGINS.md @@ -91,10 +91,15 @@ checkout unnecessary. Because the plugin ships the hooks itself, `npm run install-hooks` is not needed for plugin users — and must not be run alongside it. Events carry no id, so two -handlers mean every token and cost figure is counted twice. The bootstrap -removes the older checkout-installed entries automatically (backing -`~/.claude/settings.json` up as `settings.json.ccam-bak` first), and -`/ccam-doctor` reports the state. +handlers mean every token and cost figure is counted twice. `claude plugin +install` writes the plugin's own hook entries into `~/.claude/settings.json` +too (`${CLAUDE_PLUGIN_ROOT}` resolved to the actual cache path) — confirmed +against a real install — so those entries also contain `hook-handler.js`, same +as a leftover checkout install. The bootstrap tells them apart by whether the +command resolves under `~/.claude/plugins/cache/`: only genuine checkout paths +are removed (backing `~/.claude/settings.json` up as `settings.json.ccam-bak` +first), never the plugin's own. `/ccam-doctor` reports the state using the same +check. ### First session start diff --git a/scripts/plugin-bootstrap.js b/scripts/plugin-bootstrap.js index 76e88c9..cdf435c 100644 --- a/scripts/plugin-bootstrap.js +++ b/scripts/plugin-bootstrap.js @@ -187,11 +187,37 @@ function releaseLock(rt = runtimeDir()) { /* --------------------------------------------------------- legacy cleanup */ +/** + * `claude plugin install` materializes the plugin's inline hooks into + * `~/.claude/settings.json` itself, with `${CLAUDE_PLUGIN_ROOT}` already + * resolved to the cache path — confirmed by installing this plugin for real + * and inspecting the file. Those entries also contain `hook-handler.js`, so + * `isOurEntry()` alone (checkout vs. plugin, both match the same substring) + * cannot tell a legitimate plugin-installed hook from a leftover + * `npm run install-hooks` entry. Only entries whose command does NOT resolve + * under the plugin cache are the legacy, checkout-installed kind. + * + * @returns {(entry: object) => boolean} + */ +function isCheckoutHookEntry(claudeHome = getClaudeHome()) { + const cacheRoot = path.join(claudeHome, "plugins", "cache"); + return (entry) => { + if (!isOurEntry(entry)) return false; + const commands = [ + entry.command, + ...(Array.isArray(entry.hooks) ? entry.hooks.map((h) => h.command) : []), + ]; + return !commands.some((c) => typeof c === "string" && c.includes(cacheRoot)); + }; +} + /** * Remove hook entries a previous `npm run install-hooks` wrote into * ~/.claude/settings.json. The plugin installs its own hooks, and events carry * no id — two handlers mean every token and cost figure is counted twice. - * The original file is copied aside before any write. + * Never touches the plugin's own cache-resolved entries (see + * `isCheckoutHookEntry`) — only genuine leftover checkout paths. The original + * file is copied aside before any write. * * @returns {number} how many entries were removed */ @@ -204,10 +230,11 @@ function stripLegacyHooks(settingsPath = getSettingsPath()) { } if (!settings.hooks || typeof settings.hooks !== "object") return 0; + const isLegacy = isCheckoutHookEntry(); let removed = 0; for (const [type, entries] of Object.entries(settings.hooks)) { if (!Array.isArray(entries)) continue; - const kept = entries.filter((e) => !isOurEntry(e)); + const kept = entries.filter((e) => !isLegacy(e)); removed += entries.length - kept.length; if (kept.length) settings.hooks[type] = kept; else delete settings.hooks[type]; @@ -506,6 +533,7 @@ module.exports = { lockIsStale, acquireLock, releaseLock, + isCheckoutHookEntry, stripLegacyHooks, linkCli, installDeps, diff --git a/scripts/plugin-doctor.js b/scripts/plugin-doctor.js index 9daf2a5..91e06a9 100644 --- a/scripts/plugin-doctor.js +++ b/scripts/plugin-doctor.js @@ -14,7 +14,6 @@ const os = require("os"); const path = require("path"); const boot = require("./plugin-bootstrap"); -const { isOurEntry } = require("./install-hooks"); const { getSettingsPath, getDataDir } = require("../server/lib/claude-home"); const { mcpBuildStatus } = require("./check-mcp-build"); @@ -104,9 +103,10 @@ function countLegacyHookEntries() { try { const settings = JSON.parse(fs.readFileSync(getSettingsPath(), "utf8")); if (!settings.hooks) return 0; + const isLegacy = boot.isCheckoutHookEntry(); return Object.values(settings.hooks) .filter(Array.isArray) - .reduce((n, entries) => n + entries.filter(isOurEntry).length, 0); + .reduce((n, entries) => n + entries.filter(isLegacy).length, 0); } catch { return 0; } diff --git a/server/__tests__/plugin-bootstrap.test.js b/server/__tests__/plugin-bootstrap.test.js index 8af7c8f..5c485aa 100644 --- a/server/__tests__/plugin-bootstrap.test.js +++ b/server/__tests__/plugin-bootstrap.test.js @@ -192,6 +192,49 @@ describe("legacy hook cleanup", () => { assert.ok(fs.existsSync(`${settings}.ccam-bak`), "must back up before writing"); }); + it("never removes the plugin's own cache-resolved hook entries", () => { + // Regression: `claude plugin install` materializes the plugin's inline + // hooks into settings.json itself, with ${CLAUDE_PLUGIN_ROOT} resolved to + // the cache path — confirmed against a real install. Those entries also + // contain "hook-handler.js", so a naive substring check would strip them + // right along with genuine checkout leftovers, killing the plugin's own + // hooks on every session start. + const cachePath = path.join( + TMP_HOME, + "plugins", + "cache", + "claude-code-agent-monitor-plugins", + "ccam", + "abc123", + "scripts", + "hook-handler.js" + ); + fs.writeFileSync( + settings, + JSON.stringify({ + hooks: { + PreToolUse: [ + { + matcher: "*", + hooks: [{ type: "command", command: `node "${cachePath}" PreToolUse` }], + }, + { + matcher: "*", + hooks: [ + { type: "command", command: "node /repo/scripts/hook-handler.js PreToolUse" }, + ], + }, + ], + }, + }) + ); + const removed = boot.stripLegacyHooks(settings); + assert.equal(removed, 1); + const after = JSON.parse(fs.readFileSync(settings, "utf8")); + assert.equal(after.hooks.PreToolUse.length, 1); + assert.match(JSON.stringify(after.hooks.PreToolUse), /plugins.*cache/); + }); + it("leaves a file with no CCAM hooks untouched and writes no backup", () => { fs.writeFileSync(settings, JSON.stringify({ hooks: { Stop: [{ hooks: [] }] } })); assert.equal(boot.stripLegacyHooks(settings), 0);