fix: hooks reset no longer re-arms a disabled hook

cli/bin/commands/skills.mjs

@@ -17,7 +17,7 @@ import { get } from 'node:https';
 import { createHash } from 'node:crypto';
 import { tmpdir, homedir } from 'node:os';
 import { unzipSync } from 'fflate';
-import { getHookConsent, setHookConsent } from '../../lib/impeccable-config.mjs';
+import { getHookConsent, setHookConsent, setHookEnabled, getHookEnabled } from '../../lib/impeccable-config.mjs';
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
 const API_BASE = 'https://impeccable.style';
@@ -1658,24 +1658,38 @@ const HOOK_EXPLAINER = [
 // first time, records the answer in .impeccable/config.local.json, and never
 // re-asks: a recorded decision or an already-installed hook short-circuits, and
 // non-interactive runs keep the historical install-by-default behavior.
+// Every `true` return also affirms `hook.enabled: true` in the shared
+// config.json (issue #512): the hook runtime's DEFAULT_CONFIG.enabled
+// defaults to false, and this install path -- unlike `/impeccable hooks on`
+// -- never otherwise writes that key. Without it, an install done here wires
+// up manifests that never actually fire; idempotent to re-affirm on every run,
+// including for consent recorded before this fix existed.
+// The one thing it must never do is override an explicit `hooks off`: a
+// routine `skills install`/`update` run is not the user opting back in, so
+// `enable()` skips the write entirely when `enabled` is already explicitly
+// false, leaving a deliberate opt-out exactly as the user left it.
 async function decideHookInstall(root, targets, { yes } = {}) {
   if (targets.length === 0) return false;
+  const enable = () => {
+    if (getHookEnabled(root) !== false) setHookEnabled(root, true);
+    return true;
+  };
   const consent = getHookConsent(root);
   if (consent === 'declined') return false;
-  if (consent === 'accepted') return true;
+  if (consent === 'accepted') return enable();
   // Existing hook users (hook already wired up) are never nagged.
   if (targets.length > 0 && targets.every(provider => hookInstalledForProvider(root, provider))) {
-    return true;
+    return enable();
   }
   // Undecided and not yet installed. Non-interactive (-y or no TTY) keeps the
   // historical default-on behavior without recording a (re-promptable) decision.
-  if (yes || !process.stdin.isTTY) return true;
+  if (yes || !process.stdin.isTTY) return enable();
 
   process.stdout.write(HOOK_EXPLAINER);
   const ans = await ask('Install the design hook? (Y/n) ');
   const accepted = !(ans === 'n' || ans === 'no');
   setHookConsent(root, accepted ? 'accepted' : 'declined');
-  return accepted;
+  return accepted ? enable() : false;
 }
 
 function resolveLinkSource(sourceValue, root) {

cli/lib/impeccable-config.mjs

@@ -584,6 +584,44 @@ export function setHookConsent(root, value) {
   return filePath;
 }
 
+/**
+ * The currently effective `hook.enabled` value, mirroring the runtime's own
+ * resolution order in hook-lib.mjs's readConfig() / context.mjs's
+ * hookEnabledAt() -- shared config.json first, then config.local.json
+ * overriding it when it also has an explicit key. Returns undefined when
+ * neither file has one (the runtime treats that as disabled per
+ * DEFAULT_CONFIG, issue #512).
+ */
+export function getHookEnabled(root) {
+  let enabled;
+  for (const filePath of [getConfigPath(root), getLocalConfigPath(root)]) {
+    const hook = hookSection(safeReadJson(filePath));
+    if (hook && Object.prototype.hasOwnProperty.call(hook, 'enabled')) {
+      enabled = hook.enabled !== false;
+    }
+  }
+  return enabled;
+}
+
+/**
+ * Persist `hook.enabled` to the shared config.json, preserving any sibling
+ * keys. Mirrors skill/scripts/hook-admin.mjs's setEnabled(cwd, true): the
+ * hook runtime's DEFAULT_CONFIG.enabled defaults to false (absence of
+ * consent must not mean "on"), so this CLI install path -- the only other
+ * place manifests get written -- must explicitly opt in, or an install done
+ * through `npx impeccable skills install` would wire up manifests that never
+ * actually fire.
+ */
+export function setHookEnabled(root, value) {
+  const filePath = getConfigPath(root);
+  const existing = safeReadJson(filePath) || {};
+  const hook = hookSection(existing) || {};
+  const next = { ...existing, hook: { ...hook, enabled: value } };
+  mkdirSync(dirname(filePath), { recursive: true });
+  writeFileSync(filePath, `${JSON.stringify(next, null, 2)}\n`);
+  return filePath;
+}
+
 const EXCLUDE_OPEN = '# impeccable-config-ignore-start';
 const EXCLUDE_CLOSE = '# impeccable-config-ignore-end';
 const EXCLUDE_PATTERNS = ['.impeccable/config.local.json'];

docs/residual-review-findings/441bf5b0.md

@@ -0,0 +1,12 @@
+# Residual Review Findings
+
+Source: `ce-code-review` (correctness, testing, adversarial, project-standards) on branch `fix/512-hooks-reset-rearms-disabled-hook`, head `441bf5b0`.
+
+Applied findings are in the commit history (see `ee68b52a`, `c41bf797`, `441bf5b0`). The following were surfaced but consciously **not** applied in this PR — each is either pre-existing behavior this fix did not introduce, or disproportionate scope for a targeted bug fix.
+
+## Residual Review Findings
+
+- **P2 (adversarial, confidence 70, pre-existing)** — `skill/scripts/hook-admin.mjs:489` `stripImpeccableHookEntry` uses substring matching (`valueHasImpeccableHookMarker`) to decide whether a manifest hook entry belongs to Impeccable, and nulls the *entire* entry on a match. A hand-authored entry that chains Impeccable's hook with the user's own command in the same `command`/`args` string (e.g. `node "a.mjs" && node ".claude/skills/impeccable/scripts/hook.mjs"`) would have the user's portion silently deleted along with Impeccable's. This behavior predates this PR (both `repairHookManifests()`'s existing prune-on-reinstall path and the new `reset()` path share it) — not introduced here, and fixing it correctly (partial-string editing instead of whole-entry deletion) is a larger, separate change.
+- **P2 (adversarial, confidence 60)** — `reset()`, `setEnabled()` (`on`/`off`) all perform multiple non-atomic file reads/writes across config, cache, and manifest files with no locking. Two concurrent `hooks` invocations on the same project (two agents, or an agent and a human) could interleave and leave config/manifest state inconsistent. This is a pre-existing architectural property of the whole `hook-admin.mjs` module (not unique to `reset()`), and introducing a lock file is out of scope for this targeted fix.
+- **Advisory (adversarial residual risk)** — `fileHasImpeccableHookMarker` scans every string value under a manifest's `hooks` key for the marker substring, while `stripImpeccableHookEntry` only nulls entries whose `command`/`args`/`bash`/`powershell` fields match. A marker substring living in an unrelated field (e.g. a custom `statusMessage`) would make the prune function report `true` (rewrite the file, claim success) without actually removing anything. Low likelihood given how manifests are actually generated; noted for awareness, not actioned.
+- **Advisory (adversarial residual risk)** — `pruneImpeccableHookFromManifest` silently no-ops on a manifest that fails `JSON.parse`, with no user-visible signal. If a provider's own hook loader is more lenient (tolerates trailing commas/comments) than this admin tool's strict parser, a manifest could stay live and un-prunable with no warning. Not actioned — matches this function's pre-existing error-handling posture everywhere else it's called.

skill/scripts/context.mjs

@@ -1237,7 +1237,13 @@ function valueHasHookMarker(value) {
 
 function hookEnabledAt(root) {
   if (truthyEnv(process.env.IMPECCABLE_HOOK_DISABLED)) return false;
-  let enabled = true;
+  // Matches hook-lib.mjs's DEFAULT_CONFIG.enabled (issue #512): absence of an
+  // explicit `hook.enabled` key means disabled, not enabled. Getting this
+  // wrong here is doubly silent -- MANUAL_DETECTOR_REQUIRED (below) only
+  // fires when this function reports the hook inactive, so a stale `true`
+  // default would both skip the real hook (per hook-lib.mjs) AND suppress
+  // the fallback warning that would otherwise tell the agent to scan by hand.
+  let enabled = false;
   for (const name of ['.impeccable/config.json', '.impeccable/config.local.json']) {
     const raw = readJson(path.join(root, name));
     if (raw?.hook && Object.prototype.hasOwnProperty.call(raw.hook, 'enabled')) {

skill/scripts/hook-admin.mjs

@@ -259,7 +259,11 @@ function writeDetectorConfig(cwd, detectorConfig, opts = {}) {
 function mergeHookConfig(existing) {
   const base = existing && typeof existing === 'object' ? existing : {};
   return {
-    enabled: base.enabled === false ? false : true,
+    // Match DEFAULT_CONFIG.enabled (issue #512): absence of an explicit value
+    // means disabled, not enabled. setEnabled() overwrites this immediately
+    // for its own call site, but a future caller relying on this default
+    // alone must not silently re-arm.
+    enabled: base.enabled === true,
     limits: {
       maxFindings: Number.isFinite(base?.limits?.maxFindings) ? base.limits.maxFindings : DEFAULT_CONFIG.limits.maxFindings,
       maxChars: Number.isFinite(base?.limits?.maxChars) ? base.limits.maxChars : DEFAULT_CONFIG.limits.maxChars,
@@ -739,35 +743,123 @@ function addIgnoreValue(cwd, args) {
   return `Added ${parsed.rule}=${parsed.value}${scopeSuffix} to ${scope} (${path.relative(cwd, target) || target}).`;
 }
 
+// Best-effort restore of files reset() already mutated, used when a later
+// step in the same reset() call fails. Never throws itself -- a restore
+// failure (the underlying disk problem persisting) is reported as part of
+// the original error, not swallowed, but must not mask it with a second
+// exception.
+function restoreConfigBackups(backups) {
+  const restoreFailures = [];
+  for (const { filePath, existed, content } of backups) {
+    try {
+      if (existed) {
+        fs.writeFileSync(filePath, content);
+      } else {
+        fs.unlinkSync(filePath);
+      }
+    } catch (err) {
+      restoreFailures.push(`${filePath} (${err.message || err})`);
+    }
+  }
+  return restoreFailures;
+}
+
 function reset(cwd) {
   const removed = [];
+  const configBackups = [];
+  const configFailures = [];
   // Unified files may hold non-hook keys (e.g. updateCheck); strip only the
   // hook/detector subtrees and keep the rest, deleting the file only if nothing remains.
+  //
+  // Config + local config reset as one all-or-nothing step: back up each
+  // file's exact on-disk bytes before mutating it, so that if the second
+  // file fails after the first already succeeded, the first is rolled back
+  // rather than left reset while the second (which may still carry
+  // `hook.enabled: true`) is untouched -- a partial reset that is neither
+  // the old state nor the new one, and that manifest pruning below must
+  // never run against.
   for (const filePath of [getConfigPath(cwd), getLocalConfigPath(cwd)]) {
     try {
       const raw = readRawConfigFile(filePath).raw;
       if (!raw || typeof raw !== 'object' || Array.isArray(raw) || (!('hook' in raw) && !('detector' in raw))) continue;
       const { hook, detector, ...rest } = raw;
+      const existed = fs.existsSync(filePath);
+      configBackups.push({ filePath, existed, content: existed ? fs.readFileSync(filePath) : null });
       if (Object.keys(rest).length === 0) {
         fs.unlinkSync(filePath);
       } else {
         fs.writeFileSync(filePath, JSON.stringify(rest, null, 2) + '\n');
       }
       removed.push(path.relative(cwd, filePath) || filePath);
-    } catch { /* ignore */ }
+    } catch (err) {
+      configFailures.push(`${path.relative(cwd, filePath) || filePath} (${err.message || err})`);
+    }
   }
-  // State files are wholly ours; delete outright.
+  if (configFailures.length) {
+    // A write/unlink failure here (permissions, disk full) must not be
+    // silently swallowed: this file may still carry `hook.enabled: true`,
+    // so pruning manifests below would leave `status` reporting enabled
+    // while nothing actually invokes the hook -- reset()'s own version of
+    // the exact config/manifest mismatch issue #512 was about.
+    const restoreFailures = restoreConfigBackups(configBackups);
+    const restoreNote = restoreFailures.length
+      ? ` Additionally could not restore: ${restoreFailures.join(', ')}.`
+      : ' Restored to the prior state.';
+    throw new Error(`Could not reset hook config, so leaving manifests untouched to avoid reporting a stale enabled state: ${configFailures.join(', ')}.${restoreNote}`);
+  }
+  // State files are wholly ours; delete outright. A failure here is reported
+  // (never silently dropped, matching config/manifest handling above) but is
+  // not fatal to the rest of reset() -- unlike config, the cache/pending
+  // files are internal bookkeeping `status` never reports on, so a stale one
+  // is inert, not misleading.
+  const stateFailures = [];
   for (const filePath of [getCachePath(cwd), getPendingPath(cwd)]) {
     try {
       if (fs.existsSync(filePath)) {
         fs.unlinkSync(filePath);
         removed.push(path.relative(cwd, filePath) || filePath);
       }
-    } catch { /* ignore */ }
+    } catch (err) {
+      stateFailures.push(`${path.relative(cwd, filePath) || filePath} (${err.message || err})`);
+    }
+  }
+  // Manifests written by `hooks on` outlive config/cache removal (issue #512):
+  // with DEFAULT_CONFIG.enabled now false, a surviving manifest entry would
+  // still invoke a hook whose config no longer opts in. Prune every installed
+  // target regardless of whether its skill folder still exists on disk -- a
+  // reset mid-uninstall (skill files gone, manifest not yet cleaned) is
+  // exactly the case that most needs this, and pruneImpeccableHookFromManifest
+  // already no-ops safely on a missing or markerless file.
+  //
+  // destRel only, never sharedDestRel: `on`/repairHookManifests() only ever
+  // reads sharedDestRel (e.g. a team-committed .claude/settings.json) to
+  // decide whether it already covers the install -- it never writes there.
+  // Pruning sharedDestRel would make a single developer's local `reset` rip
+  // the hook out of a file the whole team shares, which is a materially
+  // larger blast radius than the local revocation this fix is about.
+  //
+  // pruneImpeccableHookFromManifest's own writes are not internally
+  // try/caught, so one target failing (permissions) must not abort the
+  // remaining targets or crash uncaught with no report of what did succeed.
+  const prunedManifests = [];
+  const manifestFailures = [];
+  for (const target of HOOK_MANIFEST_TARGETS) {
+    try {
+      if (pruneImpeccableHookFromManifest(path.join(cwd, target.destRel))) {
+        prunedManifests.push(target.provider);
+      }
+    } catch (err) {
+      manifestFailures.push(`${target.provider} (${err.message || err})`);
+    }
   }
-  return removed.length
-    ? `Reset design hook config and cache (removed: ${removed.join(', ')}).`
-    : 'No hook config or cache to remove. Already at defaults.';
+
+  const parts = [];
+  if (removed.length) parts.push(`Reset design hook config and cache (removed: ${removed.join(', ')}).`);
+  if (prunedManifests.length) parts.push(`Removed hook entries from: ${prunedManifests.join(', ')}.`);
+  if (stateFailures.length) parts.push(`Could not remove: ${stateFailures.join(', ')}.`);
+  if (manifestFailures.length) parts.push(`Could not prune manifests for: ${manifestFailures.join(', ')}.`);
+  if (!parts.length) return 'No hook config, cache, or manifest entries to remove. Already at defaults.';
+  return parts.join(' ');
 }
 
 function main() {

skill/scripts/hook-lib.mjs

@@ -148,7 +148,7 @@ export function isAdvisoryFinding(finding) {
 }
 
 export const DEFAULT_CONFIG = Object.freeze({
-  enabled: true,
+  enabled: false,
   quiet: false,
   auditLog: null,
   designSystem: { enabled: true },

tests/context.test.mjs

@@ -1066,10 +1066,15 @@ describe('context.mjs CLI', () => {
 
     const project = path.join(scratch, 'project');
     fs.mkdirSync(path.join(project, '.codex'), { recursive: true });
+    fs.mkdirSync(path.join(project, '.impeccable'), { recursive: true });
     fs.writeFileSync(path.join(project, 'PRODUCT.md'), '# Acme\n');
     fs.writeFileSync(path.join(project, '.codex', 'hooks.json'), JSON.stringify({
       hooks: { Stop: [{ hooks: [{ command: 'node .agents/skills/impeccable/scripts/hook.mjs' }] }] },
     }));
+    // hookEnabledAt() defaults to disabled absent explicit consent (#512); a
+    // manifest alone -- with no config recording that consent -- is no
+    // longer sufficient to count the hook as active.
+    fs.writeFileSync(path.join(project, '.impeccable', 'config.json'), JSON.stringify({ hook: { enabled: true } }));
 
     const res = spawnSync(process.execPath, [path.join(scripts, 'context.mjs')], {
       cwd: project,
@@ -1097,10 +1102,13 @@ describe('context.mjs CLI', () => {
 
     const project = path.join(scratch, 'project');
     fs.mkdirSync(path.join(project, '.cursor'), { recursive: true });
+    fs.mkdirSync(path.join(project, '.impeccable'), { recursive: true });
     fs.writeFileSync(path.join(project, 'PRODUCT.md'), '# Acme\n');
     fs.writeFileSync(path.join(project, '.cursor', 'hooks.json'), JSON.stringify({
       hooks: { preToolUse: [{ command: 'node .cursor/skills/impeccable/scripts/hook-before-edit.mjs' }] },
     }));
+    // hookEnabledAt() defaults to disabled absent explicit consent (#512).
+    fs.writeFileSync(path.join(project, '.impeccable', 'config.json'), JSON.stringify({ hook: { enabled: true } }));
 
     const res = spawnSync(process.execPath, [path.join(scripts, 'context.mjs')], {
       cwd: project,