feat(hooks): package design hook in plugin, install to settings.local.json (#243)

* feat(hooks): package design hook in plugin, install to settings.local.json Three related changes to how the Impeccable design hook is distributed, plus an unrelated build fix discovered along the way. Package the hook in the Claude Code plugin - The marketplace / `/plugin install` path previously shipped the skill and agents but no hook, so those users never got the design detector. The build now emits `plugin/hooks/hooks.json` (auto-discovered at the plugin root), resolving the script via `${CLAUDE_PLUGIN_ROOT}` so it works wherever Claude Code unpacks the plugin instead of assuming a `.claude/skills/` layout. CLI installs the hook into settings.local.json, not shared settings.json - `npx impeccable skills install/update` now writes the Claude hook to the gitignored `.claude/settings.local.json` (a machine-local install side effect) rather than the team-shared `settings.json`, which could otherwise be committed and break for teammates without the skill installed. - Graceful handling (leave-it-never-duplicate): if our hook already lives in the shared `settings.json` (a legacy install or a deliberate user move), it is honored in place and never duplicated into the local override, which would otherwise run the detector twice per edit. - The skill's `/impeccable hooks on|off` toggle is unaffected: it only writes `.impeccable/hook.json`, never the settings files. Fix universal.zip build failure under archiver v8 - `archiver` was bumped to v8 (now ESM, factory function removed) but `scripts/lib/zip.js` still used the old `archiver('zip', ...)` API, so every build silently failed to produce `dist/universal.zip` (the skill-release artifact). Switched to `new ZipArchive({...})`. Also folds in a pre-existing local rename of the hook status message ("Scanning design" -> "Checking UI changes") and its regenerated provider output. Tests: new coverage for the plugin-packaged hook manifest and the shared-settings honor-in-place path; existing CLI assertions moved to settings.local.json. Full suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cli): detect hook by marker, not file existence (Bugbot) hookInstalledForProvider treated any existing settings.local.json (or hooks.json) as proof the hook was installed. Those files commonly hold unrelated local settings, so the already-installed `skills install` path would skip repairing a genuinely missing hook that `update` would add. Detect the Impeccable marker in the file instead of mere existence. Adds a test for the exact case: a settings.local.json with only permissions still triggers hook repair and preserves the unrelated content. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(build): fail loud on a broken release zip + cover the zip writer Close the gap that let the archiver v8 break ship a 0-byte universal.zip with a green test suite: - createProviderZip no longer swallows failures. It throws on a missing source, an archive with zero entries, or a 0-byte output, and build() now exits non-zero on any such rejection. A build that can't produce its release artifact fails instead of deploying an empty bundle. - New tests/zip.test.mjs exercises the real zip writer and round-trips through extract-zip (the unpacker the CLI uses): a valid bundle unpacks to the skill tree, and the empty/missing-source cases throw. Wired into the core suite so it runs in `bun run test`. Why this matters: the prior CLI e2e tests stub the bundle as a local directory, so they never built, downloaded, or unzipped a real archive. The zip writer had no coverage and failed soft, so Dependabot's archiver 7->8 major bump merged green and the deploy shipped an unusable bundle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cli): scope hook marker scan to the hooks subtree + prune local dupes (Bugbot) Two follow-ups from Bugbot: - fileHasImpeccableHookMarker scanned the whole settings file as raw text, so an unrelated string (e.g. a permissions allow entry that mentions the hook path) could falsely read as an installed hook and block install/repair or the shared-settings skip. Now it parses the JSON and scans only the `hooks` subtree. - When the hook is honored in the shared settings.json, copyProviderHooks skipped the local write but left a stale hook in settings.local.json from an earlier machine-local install, so Claude Code loaded both and ran the detector twice per edit. It now prunes the local copy (preserving unrelated local settings, dropping the file if only our scaffolding remained). Adds tests for both: a permissions string mentioning the hook path still triggers repair, and a shared hook prunes the stale local duplicate while keeping unrelated permissions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-09-12 06:06:37 +03:00 · 2026-06-13 23:54:46 -07:00
parent 0ec64aad1b
commit 9c0012d4e1
26 changed files with 489 additions and 87 deletions
@@ -68,7 +68,12 @@ const IMPECCABLE_HOOK_COMMAND_MARKERS = [
 ];
 const PROVIDER_HOOK_ARTIFACTS = {
  '.claude': [
-    { sourceProvider: '.claude', rel: 'settings.json', destProvider: '.claude' },
+    // The hook is a machine-local install side effect, so it lands in the
+    // gitignored `.claude/settings.local.json` rather than the team-shared
+    // `settings.json`. The bundle still ships the manifest as `settings.json`
+    // (the `rel` source), but we write it to `destRel`. A hook the user moved
+    // into `settings.json` is honored in place; see copyProviderHooks.
+    { sourceProvider: '.claude', rel: 'settings.json', destProvider: '.claude', destRel: 'settings.local.json' },
  ],
  '.cursor': [
    { sourceProvider: '.cursor', rel: 'hooks.json', destProvider: '.cursor' },
@@ -434,19 +439,67 @@ function copyProviderSkills(bundleDir, root, targets) {
 }

 function hookArtifactsForProvider(bundleDir, root, provider) {
-  return (PROVIDER_HOOK_ARTIFACTS[provider] || []).map(({ sourceProvider, rel, destProvider }) => ({
-    src: join(bundleDir, sourceProvider, rel),
-    dest: join(root, destProvider, rel),
-  }));
+  return (PROVIDER_HOOK_ARTIFACTS[provider] || []).map(({ sourceProvider, rel, destProvider, destRel }) => {
+    const writeRel = destRel || rel;
+    const artifact = {
+      src: join(bundleDir, sourceProvider, rel),
+      dest: join(root, destProvider, writeRel),
+    };
+    // When the write target is a local override (e.g. settings.local.json), the
+    // team-shared sibling (settings.json) is where a legacy install or a
+    // deliberate user move would put our hook. Track it so we never duplicate.
+    if (writeRel !== rel) {
+      artifact.sharedDest = join(root, destProvider, rel);
+    }
+    return artifact;
+  });
 }

+// The file paths the CLI writes hook manifests to (the local override target,
+// e.g. settings.local.json — not the shared sibling).
 function expectedHookDests(root, providers) {
  const targets = Array.isArray(providers) ? providers : [providers];
  return targets.flatMap(provider =>
-    (PROVIDER_HOOK_ARTIFACTS[provider] || []).map(({ rel, destProvider }) => join(root, destProvider, rel))
+    (PROVIDER_HOOK_ARTIFACTS[provider] || []).map(({ rel, destProvider, destRel }) =>
+      join(root, destProvider, destRel || rel))
  );
 }

+// Whether a hook manifest file actually wires up the Impeccable hook. We parse
+// the JSON and scan only the `hooks` subtree (via valueHasImpeccableHookMarker),
+// not the raw file text: an unrelated string elsewhere — e.g. a permissions
+// allow entry that happens to mention the hook path — must not read as a hook.
+function fileHasImpeccableHookMarker(file) {
+  if (!existsSync(file)) return false;
+  let parsed;
+  try {
+    parsed = JSON.parse(readFileSync(file, 'utf-8'));
+  } catch {
+    return false;
+  }
+  if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return false;
+  if (!parsed.hooks || typeof parsed.hooks !== 'object') return false;
+  return valueHasImpeccableHookMarker(parsed.hooks);
+}
+
+// Whether our hook is already wired up for a provider, used to decide if the
+// already-installed fast path should top up a missing hook. We look for the
+// Impeccable marker — not mere file existence — because the target files
+// (settings.local.json, hooks.json) commonly hold unrelated local settings; an
+// existence check would falsely report "installed" and skip repairing a missing
+// hook that `update` would otherwise add. For Claude we also honor our hook
+// living in the shared settings.json sibling (a legacy install or user move).
+function hookInstalledForProvider(root, provider) {
+  const artifacts = PROVIDER_HOOK_ARTIFACTS[provider] || [];
+  if (artifacts.length === 0) return true;
+  return artifacts.every(({ destProvider, rel, destRel }) => {
+    const writeRel = destRel || rel;
+    if (fileHasImpeccableHookMarker(join(root, destProvider, writeRel))) return true;
+    if (writeRel !== rel && fileHasImpeccableHookMarker(join(root, destProvider, rel))) return true;
+    return false;
+  });
+}
+
 function valueHasImpeccableHookMarker(value) {
  if (typeof value === 'string') {
    return IMPECCABLE_HOOK_COMMAND_MARKERS.some(marker => value.includes(marker));
@@ -483,6 +536,46 @@ function stripImpeccableHookEntries(entries) {
    .filter(Boolean);
 }

+// Remove our hook from a manifest file, preserving any unrelated content. Used
+// when the hook is honored in the shared settings.json so a stale machine-local
+// copy doesn't make the detector run twice. Drops the file if nothing but our
+// hook scaffolding remains. Returns true if it changed anything.
+function pruneImpeccableHookFromManifest(manifestPath) {
+  if (!fileHasImpeccableHookMarker(manifestPath)) return false;
+  let parsed;
+  try {
+    parsed = JSON.parse(readFileSync(manifestPath, 'utf-8'));
+  } catch {
+    return false;
+  }
+
+  const existingHooks = parsed.hooks && typeof parsed.hooks === 'object' && !Array.isArray(parsed.hooks)
+    ? parsed.hooks
+    : {};
+  const cleanedHooks = {};
+  for (const [event, entries] of Object.entries(existingHooks)) {
+    const kept = stripImpeccableHookEntries(entries);
+    if (kept.length > 0) cleanedHooks[event] = kept;
+  }
+
+  const next = { ...parsed };
+  if (Object.keys(cleanedHooks).length > 0) {
+    next.hooks = cleanedHooks;
+  } else {
+    // Our hook was the only thing here; drop the hook-manifest scaffolding too.
+    delete next.hooks;
+    delete next.description;
+    delete next.version;
+  }
+
+  if (Object.keys(next).length === 0) {
+    rmSync(manifestPath, { force: true });
+  } else {
+    writeFileSync(manifestPath, `${JSON.stringify(next, null, 2)}\n`);
+  }
+  return true;
+}
+
 function mergeHookManifests(existing, fresh) {
  const existingObject = existing && typeof existing === 'object' && !Array.isArray(existing) ? existing : {};
  const freshObject = fresh && typeof fresh === 'object' && !Array.isArray(fresh) ? fresh : {};
@@ -520,8 +613,19 @@ function copyProviderHooks(bundleDir, root, providers, { force = false } = {}) {
  const targets = Array.isArray(providers) ? providers : [providers];
  const written = [];
  for (const provider of targets) {
-    for (const { src, dest } of hookArtifactsForProvider(bundleDir, root, provider)) {
+    for (const { src, dest, sharedDest } of hookArtifactsForProvider(bundleDir, root, provider)) {
      if (!existsSync(src)) continue;
+
+      // Leave-it-never-duplicate: our hook already lives in the team-shared
+      // settings.json (a legacy install or a deliberate user move). Honor it in
+      // place and skip the local write — but first strip any stale copy from the
+      // local override, or Claude Code would load both and run the detector
+      // twice per edit.
+      if (sharedDest && fileHasImpeccableHookMarker(sharedDest)) {
+        pruneImpeccableHookFromManifest(dest);
+        continue;
+      }
+
      const fresh = readJsonFile(src, 'Bundled hook manifest');
      let next = fresh;

@@ -691,14 +795,14 @@ async function install(flags) {
  if (existing && !force) {
    console.log(`Impeccable skills are already installed (found in ${existing}/).`);
    const targets = providersValue ? resolveInstallTargets(root, providersValue) : findInstalledProviders(root);
-    const missingHookDests = installHooks
-      ? expectedHookDests(root, targets).filter(dest => !existsSync(dest))
+    const missingHookTargets = installHooks
+      ? targets.filter(provider => !hookInstalledForProvider(root, provider))
      : [];
-    if (missingHookDests.length > 0) {
+    if (missingHookTargets.length > 0) {
      let bundleDir;
      try {
        bundleDir = await downloadAndExtractBundle();
-        const hookTargets = copyProviderHooks(bundleDir, root, targets);
+        const hookTargets = copyProviderHooks(bundleDir, root, missingHookTargets);
        if (hookTargets.length > 0) console.log(`Installed hooks into: ${hookTargets.join(', ')}`);
      } catch (e) {
        console.error(`Hook install failed: ${e.message}`);