diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index 01de1fba..0ef4a472 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -11,6 +11,8 @@ jobs: - uses: actions/setup-node@v4 with: node-version: 20 + - name: Hook installer regressions + run: node --test tests/install-hook.test.mjs - name: References, gates, fixtures, versions and links agree run: node skills/hallmark/scripts/refcheck.mjs - name: Every theme validates diff --git a/skills/hallmark/scripts/install-hook.mjs b/skills/hallmark/scripts/install-hook.mjs index 4b0a6cfd..5c94d72d 100644 --- a/skills/hallmark/scripts/install-hook.mjs +++ b/skills/hallmark/scripts/install-hook.mjs @@ -49,6 +49,7 @@ if (has("--remove") && !existsSync(settingsPath)) { const settings = readSettings(); if (!settings || typeof settings !== "object" || Array.isArray(settings)) { console.error(`refusing to rewrite ${settingsPath}: it is not a JSON object`); process.exit(1); } settings.hooks ??= {}; +if (typeof settings.hooks !== "object" || Array.isArray(settings.hooks)) { console.error(`refusing to rewrite ${settingsPath}: hooks is not a JSON object`); process.exit(1); } settings.hooks.PostToolUse ??= []; if (!Array.isArray(settings.hooks.PostToolUse)) { console.error(`refusing to rewrite ${settingsPath}: hooks.PostToolUse is not a list`); process.exit(1); } // Write beside the file and rename over it, so a crash never leaves settings.json half written. diff --git a/tests/install-hook.test.mjs b/tests/install-hook.test.mjs new file mode 100644 index 00000000..40519ec6 --- /dev/null +++ b/tests/install-hook.test.mjs @@ -0,0 +1,82 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { spawnSync } from 'node:child_process'; + +const script = resolve(dirname(fileURLToPath(import.meta.url)), '../skills/hallmark/scripts/install-hook.mjs'); +const command = `node "${resolve(dirname(script), 'lint-hook.mjs')}"`; +function fixture(t, settings) { + const dir = mkdtempSync(join(tmpdir(), 'hallmark-hook-test-')); + t.after(() => rmSync(dir, { recursive: true, force: true })); + mkdirSync(join(dir, '.claude')); + const file = join(dir, '.claude/settings.json'); + const original = JSON.stringify(settings, null, 4) + '\n'; + writeFileSync(file, original); + return { + original, + read: () => readFileSync(file, 'utf8'), + run: (...args) => spawnSync(process.execPath, [script, ...args], { cwd: dir, encoding: 'utf8' }), + }; +} + +for (const hooks of [[], [{ matcher: '*', hooks: [] }], 'invalid', 42, false]) { + for (const mode of [[], ['--remove']]) { + test(`refuses hooks ${JSON.stringify(hooks)} during ${mode.length ? 'removal' : 'installation'}`, (t) => { + const f = fixture(t, { hooks, permissions: { deny: ['Read(private.txt)'] } }); + const result = f.run(...mode); + assert.equal(result.status, 1); + assert.match(result.stderr, /hooks is not a JSON object/); + assert.doesNotMatch(result.stdout, /installed|removed/); + assert.equal(f.read(), f.original, 'invalid settings must remain byte-for-byte unchanged'); + }); + } +} + +for (const settings of [{}, { hooks: null }, { hooks: {} }]) { + test(`initializes missing hooks for ${JSON.stringify(settings)}`, (t) => { + const f = fixture(t, settings); + assert.equal(f.run().status, 0); + const result = JSON.parse(f.read()); + assert.equal(result.hooks.PostToolUse.length, 1); + assert.equal(result.hooks.PostToolUse[0].hooks[0].command, command); + }); +} + +test('installation is idempotent and preserves unrelated configuration and shared hooks', (t) => { + const other = { type: 'command', command: 'echo other' }; + const f = fixture(t, { + permissions: { deny: ['Read(private.txt)'] }, + hooks: { + PreToolUse: [{ matcher: '*', hooks: [other] }], + PostToolUse: [{ matcher: 'Write', hooks: [other, { type: 'command', command }] }], + }, + }); + assert.equal(f.run().status, 0); + const once = f.read(); + assert.equal(f.run().status, 0); + assert.equal(f.read(), once); + const result = JSON.parse(once); + assert.deepEqual(result.permissions, { deny: ['Read(private.txt)'] }); + assert.deepEqual(result.hooks.PreToolUse, [{ matcher: '*', hooks: [other] }]); + assert.deepEqual(result.hooks.PostToolUse[0], { matcher: 'Write', hooks: [other] }); + assert.equal(result.hooks.PostToolUse.length, 2); + assert.equal(f.run('--remove').status, 0); + const removed = JSON.parse(f.read()); + assert.deepEqual(removed.hooks.PostToolUse, [{ matcher: 'Write', hooks: [other] }]); + assert.deepEqual(removed.permissions, result.permissions); +}); + +test('refuses invalid PostToolUse without rewriting the settings', (t) => { + const f = fixture(t, { hooks: { PostToolUse: {} } }); + assert.equal(f.run().status, 1); + assert.equal(f.read(), f.original); +}); + +test('--print does not validate or modify existing settings', (t) => { + const f = fixture(t, { hooks: [] }); + assert.equal(f.run('--print').status, 0); + assert.equal(f.read(), f.original); +});