Fix false hook-script-missing in doctor when ${CLAUDE_PROJECT_DIR} is unexpanded

The deep staleness pass extracted a hook-script path with a greedy `\S*` prefix that swallowed the `${CLAUDE_PROJECT_DIR}/` placeholder, then existsSync'd the literal string. That string never exists, so every project installed by `impeccable hooks on` got a `hook-script-missing` finding with text claiming UI edits were going unscanned — the opposite of the truth. Split extraction from resolution. hookScriptTokenFrom now pulls the path token (quoted-first, so it handles the #399 guarded `[ ! -f "PATH" ] || node "PATH"` form and absolute user-level installs) without absorbing shell syntax. resolveHookScriptPath then applies a per-placeholder policy: - ${CLAUDE_PROJECT_DIR} expands to the scanned root (the runtime mapping). - ${CLAUDE_PLUGIN_ROOT} / ${PLUGIN_ROOT} / ${GROK_PLUGIN_ROOT}, $(...) command substitution (GitHub's $(git rev-parse)), and any other $VAR are SKIPPED: the doctor cannot know those locations and must never assert a negative it cannot verify. The check stays real: a placeholder that expands to a genuinely absent path still flags. Adds TDD coverage for every command form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-09-17 00:26:41 +03:00 · 2026-07-22 21:50:10 -07:00
parent 698a743958
commit 55094aaa0d
2 changed files with 154 additions and 9 deletions
@@ -215,12 +215,57 @@ function collectHookCommands(value, out = []) {
  return out;
 }

-// Pull the script path out of a hook command line. Commands look like
-// `node .claude/skills/impeccable/scripts/hook.mjs` and may be quoted or carry
-// trailing arguments.
-function hookScriptPathFrom(command) {
-  const match = String(command).match(/(\S*skills\/impeccable\/scripts\/hook(?:-before-edit)?\.mjs)/);
-  return match ? match[1].replace(/^['"]|['"]$/g, '') : null;
+const HOOK_MARKER = /skills\/impeccable\/scripts\/hook(?:-before-edit)?\.mjs/;
+
+// Pull the script-path token out of a hook command line, placeholders intact.
+// The forms our manifests ship:
+//   * bare:            node "${CLAUDE_PROJECT_DIR}/.../hook.mjs"
+//   * bundle-relative: node ".agents/.../hook.mjs"
+//   * legacy unquoted: node .claude/.../hook.mjs
+//   * guarded (#399):  [ ! -f "PATH" ] || node "PATH"   (PATH twice, identical)
+//   * absolute:        node "/Users/.../hook.mjs"        (user-level installs)
+//   * github portable: node "$(git rev-parse --show-toplevel)/.../hook.mjs"
+// A quoted path wins; the guard's two occurrences are identical, so the first
+// quoted match is the path. Otherwise fall back to the whitespace/metachar-
+// delimited token that ends at the marker, so we don't absorb `node`, `[`, `!`
+// or `||`. Returns the token verbatim; resolution happens separately.
+function hookScriptTokenFrom(command) {
+  const str = String(command);
+  if (!HOOK_MARKER.test(str)) return null;
+  const quoted = str.match(/"([^"]*skills\/impeccable\/scripts\/hook(?:-before-edit)?\.mjs)"/);
+  if (quoted) return quoted[1];
+  const bare = str.match(/([^\s"'|&;()]*skills\/impeccable\/scripts\/hook(?:-before-edit)?\.mjs)/);
+  return bare ? bare[1] : null;
+}
+
+// Resolve a script token to an absolute path the doctor can existsSync, or null
+// when the doctor cannot know where it points — in which case the caller must
+// NOT report it missing (a doctor never asserts a negative it cannot verify).
+//
+// Per-placeholder policy, mirroring what each runtime actually expands:
+//   ${CLAUDE_PROJECT_DIR}  → the project root being scanned. This is exactly the
+//                            runtime mapping (Claude Code sets it to the project
+//                            dir at hook time), so we EXPAND it against `root`.
+//                            Not doing so was the #402 bug: the literal
+//                            `${CLAUDE_PROJECT_DIR}/...` string never exists.
+//   ${CLAUDE_PLUGIN_ROOT}  → plugin-package install dir, set by the harness to
+//   ${PLUGIN_ROOT}           wherever the plugin/codex/grok bundle was unpacked
+//   ${GROK_PLUGIN_ROOT}      (grok aliases CLAUDE_PLUGIN_ROOT). The doctor has no
+//                            way to know that location → SKIP (return null).
+//   $(...) / backticks     → command substitution, e.g. GitHub's
+//                            `$(git rev-parse --show-toplevel)`. Not statically
+//                            resolvable → SKIP.
+//   any other ${VAR}/$VAR  → unknown to the doctor → SKIP.
+// A token with no placeholder is a literal path: absolute as-is, else relative
+// to `root`.
+function resolveHookScriptPath(token, root) {
+  if (!token) return null;
+  // Command substitution or backtick expansion we can't evaluate.
+  if (token.includes('$(') || token.includes('`')) return null;
+  const expanded = token.replace(/\$\{CLAUDE_PROJECT_DIR\}/g, root);
+  // Any placeholder or shell variable still present is one we can't map.
+  if (/\$\{[^}]*\}|\$[A-Za-z_]/.test(expanded)) return null;
+  return path.isAbsolute(expanded) ? expanded : path.join(root, expanded);
 }

 /**
@@ -246,9 +291,11 @@ export function checkHookInstallation({ projectRoot, repoRoot, providerId }) {
      installedAt = toRelative(manifestPath, projectRoot || root);

      const broken = commands.filter((command) => {
-        const scriptPath = hookScriptPathFrom(command);
-        if (!scriptPath) return false;
-        const abs = path.isAbsolute(scriptPath) ? scriptPath : path.join(root, scriptPath);
+        const token = hookScriptTokenFrom(command);
+        if (!token) return false;
+        const abs = resolveHookScriptPath(token, root);
+        // Unresolvable placeholder or command substitution: never assert missing.
+        if (!abs) return false;
        return !fs.existsSync(abs);
      });
      if (broken.length) {
@@ -272,6 +272,104 @@ describe('checkHookInstallation', () => {
      [],
    );
  });
+
+  // The manifest `impeccable hooks on` actually writes, verbatim: a
+  // `${CLAUDE_PROJECT_DIR}`-relative command. Claude Code expands the variable
+  // to the project dir at hook time; the doctor must expand it the same way
+  // (issue #402) instead of existsSync-ing the literal `${CLAUDE_PROJECT_DIR}/...`.
+  const claudeManifest = () => ({
+    hooks: {
+      PostToolUse: [{
+        matcher: 'Edit|Write|MultiEdit',
+        hooks: [{ type: 'command', command: 'node "${CLAUDE_PROJECT_DIR}/.claude/skills/impeccable/scripts/hook.mjs"' }],
+      }],
+      Stop: [{ hooks: [{ type: 'command', command: 'node "${CLAUDE_PROJECT_DIR}/.claude/skills/impeccable/scripts/hook.mjs"' }] }],
+    },
+  });
+
+  it('stays quiet for a ${CLAUDE_PROJECT_DIR} manifest when the script exists at root', () => {
+    write('.claude/skills/impeccable/scripts/hook.mjs', '// hook\n');
+    write('.claude/settings.json', JSON.stringify(claudeManifest()));
+    assert.deepEqual(
+      checkHookInstallation({ projectRoot: scratch, repoRoot: scratch, providerId: 'claude-code' }),
+      [],
+    );
+  });
+
+  it('still flags a genuinely missing script behind ${CLAUDE_PROJECT_DIR}', () => {
+    // Placeholder expands to a real path that does not exist: the check must
+    // stay real, not neutered into always-quiet.
+    write('.claude/settings.json', JSON.stringify(claudeManifest()));
+    const findings = checkHookInstallation({
+      projectRoot: scratch, repoRoot: scratch, providerId: 'claude-code',
+    });
+    assert.deepEqual(ids(findings), ['hook-script-missing']);
+  });
+
+  it('handles the #399 guarded project-relative form', () => {
+    const p = '"${CLAUDE_PROJECT_DIR}/.claude/skills/impeccable/scripts/hook.mjs"';
+    const guarded = `[ ! -f ${p} ] || node ${p}`;
+    write('.claude/settings.json', JSON.stringify({
+      hooks: { Stop: [{ hooks: [{ command: guarded }] }] },
+    }));
+    // missing → flagged
+    assert.deepEqual(
+      ids(checkHookInstallation({ projectRoot: scratch, repoRoot: scratch, providerId: 'claude-code' })),
+      ['hook-script-missing'],
+    );
+    // present → quiet
+    write('.claude/skills/impeccable/scripts/hook.mjs', '// hook\n');
+    assert.deepEqual(
+      checkHookInstallation({ projectRoot: scratch, repoRoot: scratch, providerId: 'claude-code' }),
+      [],
+    );
+  });
+
+  it('handles the #399 guarded absolute form (user-level installs)', () => {
+    const abs = path.join(scratch, '.claude', 'skills', 'impeccable', 'scripts', 'hook.mjs');
+    const p = JSON.stringify(abs);
+    const guarded = `[ ! -f ${p} ] || node ${p}`;
+    write('.claude/settings.json', JSON.stringify({
+      hooks: { Stop: [{ hooks: [{ command: guarded }] }] },
+    }));
+    // absolute path missing → flagged
+    assert.deepEqual(
+      ids(checkHookInstallation({ projectRoot: scratch, repoRoot: scratch, providerId: 'claude-code' })),
+      ['hook-script-missing'],
+    );
+    // present → quiet
+    write('.claude/skills/impeccable/scripts/hook.mjs', '// hook\n');
+    assert.deepEqual(
+      checkHookInstallation({ projectRoot: scratch, repoRoot: scratch, providerId: 'claude-code' }),
+      [],
+    );
+  });
+
+  it('never reports missing for the GitHub $(git rev-parse) form', () => {
+    // Command substitution is not statically resolvable; a doctor must not
+    // assert a negative it cannot verify.
+    write('.github/hooks/impeccable.json', JSON.stringify({
+      hooks: { postToolUse: [{ bash: 'node "$(git rev-parse --show-toplevel)/.github/skills/impeccable/scripts/hook.mjs"' }] },
+    }));
+    assert.deepEqual(
+      checkHookInstallation({ projectRoot: scratch, repoRoot: scratch, providerId: 'github' }),
+      [],
+    );
+  });
+
+  it('never reports missing for plugin-root placeholders the doctor cannot map', () => {
+    for (const token of ['${CLAUDE_PLUGIN_ROOT}', '${PLUGIN_ROOT}', '${GROK_PLUGIN_ROOT}']) {
+      fs.rmSync(path.join(scratch, '.claude'), { recursive: true, force: true });
+      write('.claude/settings.json', JSON.stringify({
+        hooks: { Stop: [{ hooks: [{ command: `node "${token}/skills/impeccable/scripts/hook.mjs"` }] }] },
+      }));
+      assert.deepEqual(
+        checkHookInstallation({ projectRoot: scratch, repoRoot: scratch, providerId: 'claude-code' }),
+        [],
+        `expected no finding for ${token}`,
+      );
+    }
+  });
 });

 // ─── retired live-mode state ───────────────────────────────────────────────