diff --git a/skill/scripts/lib/staleness-deep.mjs b/skill/scripts/lib/staleness-deep.mjs index 3276afc4d..bae03bc98 100644 --- a/skill/scripts/lib/staleness-deep.mjs +++ b/skill/scripts/lib/staleness-deep.mjs @@ -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) { diff --git a/tests/doctor.test.mjs b/tests/doctor.test.mjs index 22243396a..f46c481db 100644 --- a/tests/doctor.test.mjs +++ b/tests/doctor.test.mjs @@ -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 ───────────────────────────────────────────────