diff --git a/src/utils/plugins/pluginLoader.test.ts b/src/utils/plugins/pluginLoader.test.ts index eb2e3750f6..9848b3e686 100644 --- a/src/utils/plugins/pluginLoader.test.ts +++ b/src/utils/plugins/pluginLoader.test.ts @@ -1,13 +1,18 @@ -import { mkdtemp, mkdir, rm, symlink, writeFile } from 'fs/promises' -import { tmpdir } from 'os' -import { join, resolve } from 'path' -import { afterEach, describe, expect, test } from 'bun:test' +import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' + +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' import { setInlinePlugins } from '../../bootstrap/state.js' import type { LoadedPlugin } from '../../types/plugin.js' +import type { HooksSettings } from '../settings/types.js' +import type { PluginMarketplaceEntry } from './schemas.js' import { clearPluginCache, createPluginFromPath, + finishLoadingPluginFromPath, + mergeHooksSettings, mergePluginSources, resolveExistingPluginComponentPath, resolvePluginComponentPath, @@ -36,6 +41,60 @@ function marketplacePlugin( } } +// --------------------------------------------------------------------------- +// mergeHooksSettings — validates the marketplace supplement loader path +// (finishLoadingPluginFromPath, marketplace hook supplement block). Before +// this fix, the supplement used object spread ({...plugin.hooksConfig, +// ...entry.hooks}) which silently overwrote same-event matcher arrays from +// plugin.json with the marketplace arrays. +// --------------------------------------------------------------------------- +describe('mergeHooksSettings', () => { + test('appends marketplace matchers to plugin.json matchers for the same event', () => { + // Simulates plugin.json defining a PostToolUse hook + const pluginJsonHooks: HooksSettings = { + PostToolUse: [{ matcher: 'Bash', hooks: [{ type: 'command', command: 'plugin-json-hook' }] }], + } + // Simulates marketplace entry supplementing the same event + const marketplaceHooks: HooksSettings = { + PostToolUse: [{ matcher: 'Write', hooks: [{ type: 'command', command: 'marketplace-hook' }] }], + } + + const merged = mergeHooksSettings(pluginJsonHooks, marketplaceHooks) + + // Both matchers must be present — marketplace must not overwrite plugin.json + expect(merged.PostToolUse).toHaveLength(2) + const commands = (merged.PostToolUse as Array<{ hooks: Array<{ command?: string }> }>).map( + m => m.hooks[0]?.command, + ) + expect(commands).toContain('plugin-json-hook') + expect(commands).toContain('marketplace-hook') + }) + + test('adds marketplace event when plugin.json has no hook for it', () => { + const pluginJsonHooks: HooksSettings = { + PreToolUse: [{ matcher: 'Bash', hooks: [{ type: 'command', command: 'pre-hook' }] }], + } + const marketplaceHooks: HooksSettings = { + PostToolUse: [{ matcher: 'Write', hooks: [{ type: 'command', command: 'post-hook' }] }], + } + + const merged = mergeHooksSettings(pluginJsonHooks, marketplaceHooks) + + expect(merged.PreToolUse).toHaveLength(1) + expect(merged.PostToolUse).toHaveLength(1) + }) + + test('returns marketplace hooks when plugin.json has no hooks at all', () => { + const marketplaceHooks: HooksSettings = { + PostToolUse: [{ matcher: 'Bash', hooks: [{ type: 'command', command: 'mkt-hook' }] }], + } + + const merged = mergeHooksSettings(undefined, marketplaceHooks) + + expect(merged).toEqual(marketplaceHooks) + }) +}) + describe('mergePluginSources', () => { test('keeps the enabled copy when duplicate marketplace plugins disagree on enabled state', () => { const enabledOfficial = marketplacePlugin( @@ -445,3 +504,66 @@ describe('resolvePluginComponentPath', () => { } }) }) + +describe('finishLoadingPluginFromPath — marketplace hook supplement', () => { + let tmpDir: string + + beforeEach(async () => { + tmpDir = await mkdtemp(join(tmpdir(), 'openclaude-plugin-test-')) + }) + + afterEach(async () => { + await rm(tmpDir, { recursive: true, force: true }) + }) + + test('concatenates marketplace entry hooks with plugin.json hooks for the same event', async () => { + // Arrange: write a minimal plugin on disk with a PreToolUse hook + await mkdir(join(tmpDir, '.claude-plugin'), { recursive: true }) + await writeFile( + join(tmpDir, '.claude-plugin', 'plugin.json'), + JSON.stringify({ name: 'test-plugin', version: '1.0.0' }), + ) + await mkdir(join(tmpDir, 'hooks'), { recursive: true }) + await writeFile( + join(tmpDir, 'hooks', 'hooks.json'), + JSON.stringify({ + hooks: { + PreToolUse: [ + { matcher: 'Bash', hooks: [{ type: 'command', command: 'echo from-plugin-json' }] }, + ], + }, + }), + ) + + // Marketplace entry supplements PreToolUse with an additional matcher + const entry = { + name: 'test-plugin', + source: 'test-source', + strict: true, + hooks: { + PreToolUse: [ + { matcher: 'Write', hooks: [{ type: 'command', command: 'echo from-marketplace' }] }, + ], + }, + } as unknown as PluginMarketplaceEntry + + const errors: Parameters[3] = [] + + // Act: run the actual loader path + const plugin = await finishLoadingPluginFromPath( + entry, + 'test-plugin@marketplace', + true, + errors, + tmpDir, + ) + + // Assert: both matchers are present in order — plugin.json first, marketplace second + expect(plugin).not.toBeNull() + const preToolUse = plugin!.hooksConfig?.PreToolUse as Array<{ matcher: string }> + expect(preToolUse).toHaveLength(2) + expect(preToolUse[0]!.matcher).toBe('Bash') + expect(preToolUse[1]!.matcher).toBe('Write') + expect(errors).toHaveLength(0) + }) +}) diff --git a/src/utils/plugins/pluginLoader.ts b/src/utils/plugins/pluginLoader.ts index 4324b2603f..1aadfe3a9d 100644 --- a/src/utils/plugins/pluginLoader.ts +++ b/src/utils/plugins/pluginLoader.ts @@ -2022,9 +2022,12 @@ async function loadPluginSettings( } /** - * Merge two HooksSettings objects + * Merge two HooksSettings objects, appending matchers for shared events. + * Exported for targeted unit tests of the marketplace supplement loader path + * (see finishLoadingPluginFromPath, marketplace hook supplement block). + * Callers outside this module should not depend on this export. */ -function mergeHooksSettings( +export function mergeHooksSettings( base: HooksSettings | undefined, additional: HooksSettings, ): HooksSettings { @@ -2590,7 +2593,7 @@ async function loadPluginFromMarketplaceEntry( * entry supplementation — is identical. Extracted so the cache-only path * doesn't duplicate ~500 lines. */ -async function finishLoadingPluginFromPath( +export async function finishLoadingPluginFromPath( entry: PluginMarketplaceEntry, pluginId: string, enabled: boolean, @@ -3141,12 +3144,16 @@ async function finishLoadingPluginFromPath( } } - // Supplement hooks from marketplace entry + // Supplement hooks from marketplace entry. + // CORRECTNESS: Use mergeHooksSettings() instead of object spread so that + // per-event matcher arrays from plugin.json and the marketplace entry are + // concatenated rather than the marketplace entry silently overwriting the + // plugin.json matchers for the same event. if (entry.hooks) { - plugin.hooksConfig = { - ...(plugin.hooksConfig || {}), - ...(entry.hooks as HooksSettings), - } + plugin.hooksConfig = mergeHooksSettings( + plugin.hooksConfig, + entry.hooks as HooksSettings, + ) } }