fix(plugins): stop stripLegacyHooks from deleting the plugin's own hooks
claude plugin install materializes the plugin's inline hooks into
~/.claude/settings.json itself, with \${CLAUDE_PLUGIN_ROOT} resolved to the
actual cache path — confirmed by installing the plugin for real and
inspecting the file. Those entries also contain "hook-handler.js", so
isOurEntry()'s plain substring match could not tell a legitimate
plugin-installed hook from a leftover npm run install-hooks entry: every
SessionStart would have stripped the plugin's own working hooks right back
out. isCheckoutHookEntry() only removes entries whose command does NOT
resolve under ~/.claude/plugins/cache/. plugin-doctor.js's duplicate-hook
count uses the same predicate.
This commit is contained in:
+19
-9
@@ -1121,17 +1121,27 @@ flowchart TD
|
|||||||
### Plugin installs declare the same hooks instead
|
### Plugin installs declare the same hooks instead
|
||||||
|
|
||||||
When the dashboard is installed as the `ccam` Claude Code plugin, the eight hook
|
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`
|
entries are declared inline in `.claude-plugin/plugin.json` (each running
|
||||||
(each running `${CLAUDE_PLUGIN_ROOT}/scripts/hook-handler.js`), and
|
`${CLAUDE_PLUGIN_ROOT}/scripts/hook-handler.js`), and `install-hooks.js` is not
|
||||||
`install-hooks.js` is not used at all. Both at once would POST every event twice
|
used at all. `claude plugin install` itself materializes these into
|
||||||
— events carry no id, so ingest cannot deduplicate them, and every token and
|
`~/.claude/settings.json` with `${CLAUDE_PLUGIN_ROOT}` already resolved to the
|
||||||
cost figure would double. Two guards keep that from happening silently:
|
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
|
- `scripts/plugin-bootstrap.js` strips checkout-style `hook-handler.js` entries
|
||||||
`~/.claude/settings.json` on session start (backing the file up as
|
out of `~/.claude/settings.json` on session start (backing the file up as
|
||||||
`settings.json.ccam-bak` first) and logs what it removed.
|
`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
|
- `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
|
`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
|
Node >= 22.5 gate (`node:sqlite`), an atomic `mkdir` lock, the dependency
|
||||||
|
|||||||
+9
-4
@@ -91,10 +91,15 @@ checkout unnecessary.
|
|||||||
|
|
||||||
Because the plugin ships the hooks itself, `npm run install-hooks` is not needed
|
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
|
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
|
handlers mean every token and cost figure is counted twice. `claude plugin
|
||||||
removes the older checkout-installed entries automatically (backing
|
install` writes the plugin's own hook entries into `~/.claude/settings.json`
|
||||||
`~/.claude/settings.json` up as `settings.json.ccam-bak` first), and
|
too (`${CLAUDE_PLUGIN_ROOT}` resolved to the actual cache path) — confirmed
|
||||||
`/ccam-doctor` reports the state.
|
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
|
### First session start
|
||||||
|
|
||||||
|
|||||||
@@ -187,11 +187,37 @@ function releaseLock(rt = runtimeDir()) {
|
|||||||
|
|
||||||
/* --------------------------------------------------------- legacy cleanup */
|
/* --------------------------------------------------------- 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
|
* Remove hook entries a previous `npm run install-hooks` wrote into
|
||||||
* ~/.claude/settings.json. The plugin installs its own hooks, and events carry
|
* ~/.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.
|
* 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
|
* @returns {number} how many entries were removed
|
||||||
*/
|
*/
|
||||||
@@ -204,10 +230,11 @@ function stripLegacyHooks(settingsPath = getSettingsPath()) {
|
|||||||
}
|
}
|
||||||
if (!settings.hooks || typeof settings.hooks !== "object") return 0;
|
if (!settings.hooks || typeof settings.hooks !== "object") return 0;
|
||||||
|
|
||||||
|
const isLegacy = isCheckoutHookEntry();
|
||||||
let removed = 0;
|
let removed = 0;
|
||||||
for (const [type, entries] of Object.entries(settings.hooks)) {
|
for (const [type, entries] of Object.entries(settings.hooks)) {
|
||||||
if (!Array.isArray(entries)) continue;
|
if (!Array.isArray(entries)) continue;
|
||||||
const kept = entries.filter((e) => !isOurEntry(e));
|
const kept = entries.filter((e) => !isLegacy(e));
|
||||||
removed += entries.length - kept.length;
|
removed += entries.length - kept.length;
|
||||||
if (kept.length) settings.hooks[type] = kept;
|
if (kept.length) settings.hooks[type] = kept;
|
||||||
else delete settings.hooks[type];
|
else delete settings.hooks[type];
|
||||||
@@ -506,6 +533,7 @@ module.exports = {
|
|||||||
lockIsStale,
|
lockIsStale,
|
||||||
acquireLock,
|
acquireLock,
|
||||||
releaseLock,
|
releaseLock,
|
||||||
|
isCheckoutHookEntry,
|
||||||
stripLegacyHooks,
|
stripLegacyHooks,
|
||||||
linkCli,
|
linkCli,
|
||||||
installDeps,
|
installDeps,
|
||||||
|
|||||||
@@ -14,7 +14,6 @@ const os = require("os");
|
|||||||
const path = require("path");
|
const path = require("path");
|
||||||
|
|
||||||
const boot = require("./plugin-bootstrap");
|
const boot = require("./plugin-bootstrap");
|
||||||
const { isOurEntry } = require("./install-hooks");
|
|
||||||
const { getSettingsPath, getDataDir } = require("../server/lib/claude-home");
|
const { getSettingsPath, getDataDir } = require("../server/lib/claude-home");
|
||||||
const { mcpBuildStatus } = require("./check-mcp-build");
|
const { mcpBuildStatus } = require("./check-mcp-build");
|
||||||
|
|
||||||
@@ -104,9 +103,10 @@ function countLegacyHookEntries() {
|
|||||||
try {
|
try {
|
||||||
const settings = JSON.parse(fs.readFileSync(getSettingsPath(), "utf8"));
|
const settings = JSON.parse(fs.readFileSync(getSettingsPath(), "utf8"));
|
||||||
if (!settings.hooks) return 0;
|
if (!settings.hooks) return 0;
|
||||||
|
const isLegacy = boot.isCheckoutHookEntry();
|
||||||
return Object.values(settings.hooks)
|
return Object.values(settings.hooks)
|
||||||
.filter(Array.isArray)
|
.filter(Array.isArray)
|
||||||
.reduce((n, entries) => n + entries.filter(isOurEntry).length, 0);
|
.reduce((n, entries) => n + entries.filter(isLegacy).length, 0);
|
||||||
} catch {
|
} catch {
|
||||||
return 0;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -192,6 +192,49 @@ describe("legacy hook cleanup", () => {
|
|||||||
assert.ok(fs.existsSync(`${settings}.ccam-bak`), "must back up before writing");
|
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", () => {
|
it("leaves a file with no CCAM hooks untouched and writes no backup", () => {
|
||||||
fs.writeFileSync(settings, JSON.stringify({ hooks: { Stop: [{ hooks: [] }] } }));
|
fs.writeFileSync(settings, JSON.stringify({ hooks: { Stop: [{ hooks: [] }] } }));
|
||||||
assert.equal(boot.stripLegacyHooks(settings), 0);
|
assert.equal(boot.stripLegacyHooks(settings), 0);
|
||||||
|
|||||||
Reference in New Issue
Block a user