Fix: make hooks reset remove installed manifest entries (#668)

Fixes #512 and resolves the verified reproduction in #667.\n\nAI-assisted change: reviewed and merged by Codex under maintainer direction.

skill/reference/hooks.md

@@ -32,7 +32,7 @@ The first argument is the action. Defaults to `status`.
 | `ignore-value <id> <value> [--shared] [--reason "..."]` | Append a rule/value suppression to shared `.impeccable/config.json`. |
 | `ignore-value <id> <value> --local [--reason "..."]` | Append a private rule/value suppression to `.impeccable/config.local.json`. |
 | `ignore-value <id> "*" --file <glob> [--file <glob>...]` | Turn one rule off in matching files only, leaving it active everywhere else. Repeat `--file`, or use `--file=<glob>` / `--files=<glob>`. A bare `"*"` with no `--file` is refused: use `ignore-rule <id>` 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
 

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() {

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()', () => {