diff --git a/skill/reference/hooks.md b/skill/reference/hooks.md index 739a7386e..20311e938 100644 --- a/skill/reference/hooks.md +++ b/skill/reference/hooks.md @@ -32,7 +32,7 @@ The first argument is the action. Defaults to `status`. | `ignore-value [--shared] [--reason "..."]` | Append a rule/value suppression to shared `.impeccable/config.json`. | | `ignore-value --local [--reason "..."]` | Append a private rule/value suppression to `.impeccable/config.local.json`. | | `ignore-value "*" --file [--file ...]` | Turn one rule off in matching files only, leaving it active everywhere else. Repeat `--file`, or use `--file=` / `--files=`. A bare `"*"` with no `--file` is refused: use `ignore-rule ` if you really mean project-wide. | -| `reset` | Delete the project config, dedup cache, and Cursor pending queue. | +| `reset` | Delete the project config, dedup cache, and Cursor pending queue, and remove the hook's entries from every provider manifest `on` installs, the committed Copilot file included (a team-shared `settings.json` that `on` never writes is never touched). | ## Flow diff --git a/skill/scripts/hook-admin.mjs b/skill/scripts/hook-admin.mjs index 28677c483..b8af51975 100644 --- a/skill/scripts/hook-admin.mjs +++ b/skill/scripts/hook-admin.mjs @@ -770,9 +770,22 @@ function reset(cwd) { } } catch { /* ignore */ } } - return removed.length - ? `Reset design hook config and cache (removed: ${removed.join(', ')}).` - : 'No hook config or cache to remove. Already at defaults.'; + // `on` writes three things: config, consent, and hook entries in the + // provider manifests. Reset must undo all three (issue #512): a leftover + // manifest entry kept invoking the hook after the config that said "off" + // was deleted. Local destRel only, since `on` never writes the team-shared + // sharedDestRel. No skill-folder gate: a reset mid-uninstall (skill files + // gone, manifest still wired) is the case that most needs the prune. + const pruned = []; + for (const target of HOOK_MANIFEST_TARGETS) { + try { + if (pruneImpeccableHookFromManifest(path.join(cwd, target.destRel))) pruned.push(target.provider); + } catch { /* ignore */ } + } + const parts = []; + if (removed.length) parts.push(`Reset design hook config and cache (removed: ${removed.join(', ')}).`); + if (pruned.length) parts.push(`Removed hook entries from: ${pruned.join(', ')}.`); + return parts.length ? parts.join(' ') : 'No hook config or cache to remove. Already at defaults.'; } function main() { diff --git a/tests/hook.test.mjs b/tests/hook.test.mjs index 4719573eb..ee12a5876 100644 --- a/tests/hook.test.mjs +++ b/tests/hook.test.mjs @@ -1311,6 +1311,109 @@ describe('hook-admin.mjs', () => { assert.equal(r.stdout, ''); assert.equal(r.audit.skipped, 'config-ignore-file'); }); + + it('reset prunes impeccable entries from an installed manifest, sibling entries survive', () => { + // Deliberately no .claude/skills/ folder in this fixture: the prune must + // not be gated on the skill install surviving (repairHookManifests() + // gates on it; a reset mid-uninstall most needs the prune to run anyway). + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: false } })); + fs.mkdirSync(path.join(cwd, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.local.json'), JSON.stringify({ + description: 'Impeccable design detector', + hooks: { + PostToolUse: [ + { matcher: 'OtherTool', hooks: [{ type: 'command', command: 'node "./local-hook.mjs"' }] }, + { matcher: 'Edit|Write', hooks: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }, + ], + Stop: [{ hooks: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }], + }, + })); + + const out = runAdmin(['reset']); + assert.match(out, /Reset design hook config and cache \(removed:/); + assert.match(out, /Removed hook entries from: \.claude\./); + + const claude = JSON.parse(fs.readFileSync(path.join(cwd, '.claude', 'settings.local.json'), 'utf-8')); + assert.equal(claude.hooks.PostToolUse.length, 1); + assert.match(claude.hooks.PostToolUse[0].hooks[0].command, /local-hook\.mjs/); + assert.equal(claude.hooks.Stop, undefined, 'the impeccable-only Stop array should be dropped entirely'); + }); + + it('reset prunes all four provider manifests installed via `on`', () => { + for (const provider of ['.claude', '.agents', '.cursor', '.github']) { + fs.mkdirSync(path.join(cwd, provider, 'skills', 'impeccable', 'scripts'), { recursive: true }); + } + runAdmin(['on']); + assert.match(fs.readFileSync(path.join(cwd, '.claude', 'settings.local.json'), 'utf-8'), /skills\/impeccable\/scripts\/hook\.mjs/); + + const out = runAdmin(['reset']); + assert.match(out, /Reset design hook config and cache \(removed:/); + assert.match(out, /Removed hook entries from: \.claude, \.agents, \.cursor, \.github\./); + + assert.equal(fs.existsSync(path.join(cwd, '.claude', 'settings.local.json')), false, 'nothing else was in the manifest, so it is removed entirely'); + assert.equal(fs.existsSync(path.join(cwd, '.codex', 'hooks.json')), false); + assert.equal(fs.existsSync(path.join(cwd, '.cursor', 'hooks.json')), false); + assert.equal(fs.existsSync(path.join(cwd, '.github', 'hooks', 'impeccable.json')), false); + }); + + it('reset never touches the shared/committed manifest, only the local one', () => { + // .claude/settings.json is the team-shared, typically committed file; + // `on` only ever reads it and never writes it, so reset honors the same + // write-scope asymmetry. + const shared = JSON.stringify({ + hooks: { PostToolUse: [{ matcher: 'Edit', hooks: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }] }, + }); + fs.mkdirSync(path.join(cwd, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.json'), shared); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.local.json'), shared); + + const out = runAdmin(['reset']); + + // No config existed, so the message carries only the prune half. + assert.match(out, /Removed hook entries from: \.claude\./); + assert.doesNotMatch(out, /Reset design hook config and cache/); + assert.equal(fs.readFileSync(path.join(cwd, '.claude', 'settings.json'), 'utf-8'), shared, 'shared settings.json must survive reset untouched'); + assert.equal(fs.existsSync(path.join(cwd, '.claude', 'settings.local.json')), false, 'the local settings.local.json is still pruned'); + }); + + it('reset with no manifests installed keeps the original config-only message', () => { + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { enabled: false } })); + + const out = runAdmin(['reset']); + assert.match(out, /Reset design hook config and cache/); + assert.doesNotMatch(out, /Removed hook entries from/); + assert.equal(fs.existsSync(getConfigPath(cwd)), false); + }); + + it('reset leaves a manifest with no impeccable marker byte-for-byte unchanged', () => { + fs.mkdirSync(path.join(cwd, '.claude'), { recursive: true }); + const unrelated = JSON.stringify({ + hooks: { PostToolUse: [{ matcher: 'OtherTool', hooks: [{ type: 'command', command: 'node "./unrelated.mjs"' }] }] }, + }); + fs.writeFileSync(path.join(cwd, '.claude', 'settings.local.json'), unrelated); + + const out = runAdmin(['reset']); + + assert.match(out, /Already at defaults/); + assert.equal(fs.readFileSync(path.join(cwd, '.claude', 'settings.local.json'), 'utf-8'), unrelated); + }); + + it('on, off, then reset leaves nothing armed: config gone and every manifest unwired (issue #512 repro)', () => { + fs.mkdirSync(path.join(cwd, '.claude', 'skills', 'impeccable', 'scripts'), { recursive: true }); + runAdmin(['on']); + runAdmin(['off']); + + runAdmin(['reset']); + + // The harness only invokes the hook through a manifest entry, so with the + // config and every manifest gone the project cannot re-arm, whatever the + // config default says. + assert.equal(fs.existsSync(getConfigPath(cwd)), false); + assert.equal(fs.existsSync(getLocalConfigPath(cwd)), false); + assert.equal(fs.existsSync(path.join(cwd, '.claude', 'settings.local.json')), false); + }); }); describe('renderTemplate()', () => {