Skip to content

Commit 0f321a8

Browse files
committed
feat(mcp): add hook management tools to the MCP server
Adds list_hooks, list_hook_runs, create/update/delete/toggle_local_hook, and run_hook MCP tools so an agent can inspect and manage a workspace's lifecycle hooks without going through the app UI. Local-hook writes and run_hook reuse the exact same validation/execution paths as the existing HOOK_CREATE/UPDATE/ DELETE/TOGGLE IPC handlers and the Run Hook dialog. Repo hooks stay read-only and there is no trust-granting tool anywhere in the MCP surface — run_hook refuses to execute an untrusted repo hook, matching the app's own gating. Closes #153
1 parent 9f3738c commit 0f321a8

11 files changed

Lines changed: 751 additions & 116 deletions

File tree

app/src/main/ipc/__tests__/hooks.test.ts

Lines changed: 171 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,18 @@ vi.mock('../../telemetry.js', () => ({ log: { error: vi.fn(), info: vi.fn(), war
88

99
import { openConfigDb, openWorkspaceDb, type ConfigDb } from '@sproutgit/database';
1010
import { hookDefinitions, hookDependencies } from '@sproutgit/database/schema/workspace';
11-
import { getEffectiveHooks } from '../hooks.js';
11+
import {
12+
getEffectiveHooks,
13+
createLocalHook,
14+
updateLocalHook,
15+
deleteLocalHook,
16+
toggleLocalHook,
17+
runHookForMcp,
18+
} from '../hooks.js';
1219
import { writeLocalHooksFile, localHooksFilePath, repoHooksFilePath, hashHookDefinition, readHooksFile } from '../../hooks-file.js';
13-
import { trustHook } from '../../hooks-trust.js';
20+
import { trustHook, isHookTrusted } from '../../hooks-trust.js';
1421
import type { HookFileDefinition } from '@sproutgit/types';
22+
import type { BrowserWindow } from 'electron';
1523

1624
function workspaceDbPath(workspacePath: string): string {
1725
return join(workspacePath, '.sproutgit', 'state.db');
@@ -158,3 +166,164 @@ describe('getEffectiveHooks', () => {
158166
expect(result.hooks).toEqual([]);
159167
});
160168
});
169+
170+
/** Matches the shape createLocalHook/updateLocalHook expect — a fresh object per call since dependsOn/name get mutated by individual tests. */
171+
function localHookInput(overrides: { workspacePath: string } & Partial<Parameters<typeof createLocalHook>[0]>): Parameters<typeof createLocalHook>[0] {
172+
return {
173+
name: 'Install deps', scope: 'worktree', trigger: 'after_worktree_create',
174+
executionTarget: 'trigger_worktree', shell: 'bash', script: 'pnpm install',
175+
enabled: true, critical: false, switchOncePerSession: false, switchRunOnCreate: true,
176+
switchRunOnDelete: false, keepOpenOnCompletion: false, timeoutSeconds: 60, dependsOn: [],
177+
...overrides,
178+
};
179+
}
180+
181+
describe('local hook CRUD — reused as-is by the MCP write tools (see mcp-bridge.ts)', () => {
182+
let workspacePath: string;
183+
184+
beforeEach(() => {
185+
workspacePath = mkdtempSync(join(tmpdir(), 'sg-hooks-crud-test-'));
186+
});
187+
188+
afterEach(() => {
189+
rmSync(workspacePath, { recursive: true, force: true });
190+
});
191+
192+
it('createLocalHook adds a hook to local-hooks.json', () => {
193+
createLocalHook(localHookInput({ workspacePath }));
194+
const { hooks } = readHooksFile(localHooksFilePath(workspacePath));
195+
expect(hooks).toHaveLength(1);
196+
expect(hooks[0]?.name).toBe('Install deps');
197+
});
198+
199+
it('createLocalHook rejects a duplicate local hook name', () => {
200+
createLocalHook(localHookInput({ workspacePath }));
201+
expect(() => createLocalHook(localHookInput({ workspacePath }))).toThrow(/already exists/);
202+
});
203+
204+
it('createLocalHook rejects a dependsOn referencing an unknown hook', () => {
205+
expect(() => createLocalHook(localHookInput({ workspacePath, dependsOn: ['does-not-exist'] })))
206+
.toThrow(/unknown local hook/);
207+
// The rejected hook must never have been written — otherwise it would
208+
// corrupt local-hooks.json and disable every local hook on next read.
209+
const { hooks } = readHooksFile(localHooksFilePath(workspacePath));
210+
expect(hooks).toHaveLength(0);
211+
});
212+
213+
it('createLocalHook rejects a hook that depends on itself', () => {
214+
expect(() => createLocalHook(localHookInput({ workspacePath, name: 'a', dependsOn: ['a'] })))
215+
.toThrow(/cannot depend on itself/);
216+
});
217+
218+
it('updateLocalHook renames a hook and cascades the rename into other hooks\' dependsOn', () => {
219+
createLocalHook(localHookInput({ workspacePath, name: 'a' }));
220+
createLocalHook(localHookInput({ workspacePath, name: 'b', dependsOn: ['a'] }));
221+
updateLocalHook({ workspacePath, id: 'local:a', name: 'a-renamed' });
222+
const { hooks } = readHooksFile(localHooksFilePath(workspacePath));
223+
expect(hooks.find(h => h.name === 'b')?.dependsOn).toEqual(['a-renamed']);
224+
});
225+
226+
it('updateLocalHook rejects a dependsOn referencing an unknown hook, leaving the file untouched', () => {
227+
createLocalHook(localHookInput({ workspacePath, name: 'a' }));
228+
expect(() => updateLocalHook({ workspacePath, id: 'local:a', dependsOn: ['does-not-exist'] }))
229+
.toThrow(/unknown local hook/);
230+
const { hooks } = readHooksFile(localHooksFilePath(workspacePath));
231+
expect(hooks.find(h => h.name === 'a')?.dependsOn).toEqual([]);
232+
});
233+
234+
it('updateLocalHook rejects a rename that would make dependsOn reference itself', () => {
235+
createLocalHook(localHookInput({ workspacePath, name: 'a', dependsOn: [] }));
236+
createLocalHook(localHookInput({ workspacePath, name: 'b', dependsOn: ['a'] }));
237+
// Renaming "a" to "b" while "b" already depends on the name "a" would
238+
// leave "b" depending on itself post-rename.
239+
expect(() => updateLocalHook({ workspacePath, id: 'local:b', dependsOn: ['b'] }))
240+
.toThrow(/cannot depend on itself/);
241+
});
242+
243+
it('updateLocalHook is a no-op for a repo hook id — repo hooks stay read-only', () => {
244+
createLocalHook(localHookInput({ workspacePath }));
245+
updateLocalHook({ workspacePath, id: 'repo:Install deps', enabled: false });
246+
const { hooks } = readHooksFile(localHooksFilePath(workspacePath));
247+
expect(hooks[0]?.enabled).toBe(true);
248+
});
249+
250+
it('deleteLocalHook removes the hook and clears dangling dependsOn references', () => {
251+
createLocalHook(localHookInput({ workspacePath, name: 'a' }));
252+
createLocalHook(localHookInput({ workspacePath, name: 'b', dependsOn: ['a'] }));
253+
deleteLocalHook({ workspacePath, id: 'local:a' });
254+
const { hooks } = readHooksFile(localHooksFilePath(workspacePath));
255+
expect(hooks.map(h => h.name)).toEqual(['b']);
256+
expect(hooks[0]?.dependsOn).toEqual([]);
257+
});
258+
259+
it('deleteLocalHook is a no-op for a repo hook id', () => {
260+
createLocalHook(localHookInput({ workspacePath }));
261+
deleteLocalHook({ workspacePath, id: 'repo:Install deps' });
262+
const { hooks } = readHooksFile(localHooksFilePath(workspacePath));
263+
expect(hooks).toHaveLength(1);
264+
});
265+
266+
it('toggleLocalHook flips enabled for a local hook', () => {
267+
createLocalHook(localHookInput({ workspacePath }));
268+
toggleLocalHook({ workspacePath, id: 'local:Install deps', enabled: false });
269+
const { hooks } = readHooksFile(localHooksFilePath(workspacePath));
270+
expect(hooks[0]?.enabled).toBe(false);
271+
});
272+
});
273+
274+
describe('runHookForMcp — the run_hook tool boundary (no trust granting, no bypass)', () => {
275+
let workspacePath: string;
276+
let worktreePath: string;
277+
let configDb: ConfigDb;
278+
const fakeWin = { webContents: { send: () => { /* no-op */ } } } as unknown as BrowserWindow;
279+
280+
beforeEach(() => {
281+
workspacePath = mkdtempSync(join(tmpdir(), 'sg-hooks-run-test-'));
282+
worktreePath = mkdtempSync(join(tmpdir(), 'sg-hooks-run-worktree-'));
283+
configDb = openConfigDb(join(workspacePath, 'config.db'));
284+
});
285+
286+
afterEach(() => {
287+
configDb.close();
288+
rmSync(workspacePath, { recursive: true, force: true });
289+
rmSync(worktreePath, { recursive: true, force: true });
290+
});
291+
292+
it('refuses to run an untrusted repo hook, and grants it no trust as a side effect', async () => {
293+
writeFileSync(repoHooksFilePath(worktreePath), JSON.stringify({
294+
version: 1,
295+
hooks: [{ name: 'Repo hook', trigger: 'manual', executionTarget: 'trigger_worktree', shell: 'bash', script: 'echo pwned' }],
296+
}));
297+
const { hooks: repoDefs } = readHooksFile(repoHooksFilePath(worktreePath));
298+
const hash = hashHookDefinition(repoDefs[0]!);
299+
expect(isHookTrusted(configDb, worktreePath, hash)).toBe(false);
300+
301+
const result = await runHookForMcp({ workspacePath, hookId: 'repo:Repo hook', worktreePath }, fakeWin, configDb);
302+
303+
expect(result.status).toBe('not_run');
304+
expect(result.errorMessage).toMatch(/untrusted/i);
305+
// The whole point of this test: calling run_hook must never have the
306+
// side effect of trusting the hook it refused to run.
307+
expect(isHookTrusted(configDb, worktreePath, hash)).toBe(false);
308+
});
309+
310+
it('refuses to run a disabled local hook', async () => {
311+
createLocalHook(localHookInput({ workspacePath, name: 'Off hook', enabled: false, trigger: 'manual' }));
312+
const result = await runHookForMcp({ workspacePath, hookId: 'local:Off hook', worktreePath }, fakeWin, configDb);
313+
expect(result.status).toBe('not_run');
314+
expect(result.errorMessage).toMatch(/disabled/i);
315+
});
316+
317+
it('reports not_run with a clear message for an unknown hookId', async () => {
318+
const result = await runHookForMcp({ workspacePath, hookId: 'local:does-not-exist', worktreePath }, fakeWin, configDb);
319+
expect(result.status).toBe('not_run');
320+
expect(result.errorMessage).toMatch(/no hook/i);
321+
});
322+
323+
it('reports not_run when no workspace window is open, rather than throwing', async () => {
324+
createLocalHook(localHookInput({ workspacePath, name: 'Trusted hook', trigger: 'manual' }));
325+
const result = await runHookForMcp({ workspacePath, hookId: 'local:Trusted hook', worktreePath }, null, configDb);
326+
expect(result.status).toBe('not_run');
327+
expect(result.errorMessage).toMatch(/no open window/i);
328+
});
329+
});

0 commit comments

Comments
 (0)