diff --git a/.codex/hooks.json b/.codex/hooks.json index ac6b57353..dfcda438b 100644 --- a/.codex/hooks.json +++ b/.codex/hooks.json @@ -6,7 +6,7 @@ "hooks": [ { "type": "command", - "command": "[ ! -f \".agents/skills/impeccable/scripts/hook.mjs\" ] || node \".agents/skills/impeccable/scripts/hook.mjs\"", + "command": "[ ! -f \".codex/skills/impeccable/scripts/hook.mjs\" ] || node \".codex/skills/impeccable/scripts/hook.mjs\"", "timeout": 5, "statusMessage": "Checking UI changes" } @@ -18,7 +18,7 @@ "hooks": [ { "type": "command", - "command": "[ ! -f \".agents/skills/impeccable/scripts/hook.mjs\" ] || node \".agents/skills/impeccable/scripts/hook.mjs\"", + "command": "[ ! -f \".codex/skills/impeccable/scripts/hook.mjs\" ] || node \".codex/skills/impeccable/scripts/hook.mjs\"", "timeout": 30, "statusMessage": "Design deep pass" } diff --git a/cli/bin/commands/skills.mjs b/cli/bin/commands/skills.mjs index 5e9b033a9..07fd91e44 100644 --- a/cli/bin/commands/skills.mjs +++ b/cli/bin/commands/skills.mjs @@ -1179,6 +1179,19 @@ function hookArtifactsForProvider(bundleDir, root, provider) { }); } +// The project-relative hook command path for a provider, used for project-scope +// installs (skillRoot === root). Derived rather than copied from the bundle: the +// Codex bundle ships a `.codex/skills/...` command (correct for a `.codex`- +// directory install), but the CLI lays Codex's skill down at `.agents/skills/`, +// so preserving the bundle token would point the hook at a nonexistent file and +// silently no-op it. Claude keeps its ${CLAUDE_PROJECT_DIR} token so a manifest +// read from a nested cwd (or copied into settings.local.json) still resolves. +function hookScriptRelPathForProvider(provider) { + const script = provider === '.cursor' ? 'hook-before-edit.mjs' : 'hook.mjs'; + const rel = `${provider}/skills/impeccable/scripts/${script}`; + return provider === '.claude' ? '${CLAUDE_PROJECT_DIR}/' + rel : rel; +} + function hookScriptPathForProvider(skillRoot, provider) { // `.github` is intentionally absent: its hook manifest (`.github/hooks/ // impeccable.json`) is a committed, team-shared file that the Copilot cloud @@ -1228,9 +1241,10 @@ function rewriteHookCommandsForSkillRoot(value, provider, { skillRoot, absolute if (absolute) { quotedPath = JSON.stringify(hookScript); } else { - // Preserve the bundle's own path token (e.g. ${CLAUDE_PROJECT_DIR}/...). - const match = value.match(/"([^"]+)"/); - quotedPath = match ? JSON.stringify(match[1]) : JSON.stringify(hookScript); + // Project-scope install: derive the provider's own project-relative path + // rather than trusting the bundle token, which for Codex points at + // `.codex/skills/...` while the CLI installs the skill at `.agents/skills/`. + quotedPath = JSON.stringify(hookScriptRelPathForProvider(provider)); } return guardHookCommand(quotedPath); } diff --git a/scripts/build.js b/scripts/build.js index de3ff3e60..e099eb280 100644 --- a/scripts/build.js +++ b/scripts/build.js @@ -400,7 +400,7 @@ function syncRootHookManifests(rootDir) { const synced = []; for (const config of Object.values(PROVIDERS)) { if (!config.emitHooks) continue; - const manifest = hooksJsonFor(config.emitHooks); + const manifest = hooksJsonFor(config.emitHooks, { configDir: config.configDir }); if (!manifest) continue; const rel = config.hooksManifestRel || path.join('hooks', 'hooks.json'); const dest = path.join(rootDir, config.configDir, rel); diff --git a/scripts/lib/transformers/factory.js b/scripts/lib/transformers/factory.js index 788542914..6b92a6aa2 100644 --- a/scripts/lib/transformers/factory.js +++ b/scripts/lib/transformers/factory.js @@ -308,7 +308,7 @@ export function createTransformer(config) { // `.codex/hooks.json`, and Cursor uses `.cursor/hooks.json`. let hooksEmitted = false; if (config.emitHooks) { - const manifest = hooksJsonFor(config.emitHooks); + const manifest = hooksJsonFor(config.emitHooks, { configDir }); if (manifest) { const hooksRel = config.hooksManifestRel || path.join('hooks', 'hooks.json'); writeFile(path.join(providerDir, configDir, hooksRel), JSON.stringify(manifest, null, 2) + '\n'); diff --git a/scripts/lib/transformers/hooks.js b/scripts/lib/transformers/hooks.js index f21f3dfaa..95da1ed6c 100644 --- a/scripts/lib/transformers/hooks.js +++ b/scripts/lib/transformers/hooks.js @@ -54,7 +54,13 @@ const CLAUDE_PROJECT_HOOK = '${CLAUDE_PROJECT_DIR}/.claude/skills/impeccable/scr const guardedNode = (hookPath) => `[ ! -f "${hookPath}" ] || node "${hookPath}"`; const CLAUDE_PLUGIN_HOOK = '${CLAUDE_PLUGIN_ROOT}/skills/impeccable/scripts/hook.mjs'; const CODEX_PLUGIN_HOOK = '${PLUGIN_ROOT}/skills/impeccable/scripts/hook.mjs'; -const CODEX_PROJECT_HOOK = '.agents/skills/impeccable/scripts/hook.mjs'; +// Codex reads project hooks from `.codex/hooks.json`, but the skill payload the +// hook invokes lives under the install's own skills dir: a `.codex`-directory +// install keeps it at `.codex/skills/...`, while a `.agents` (Codex repo-skills) +// install keeps it at `.agents/skills/...`. Derive the path from the install dir +// so each generated manifest points at its own payload rather than a hardcoded +// `.agents` — otherwise the guarded hook silently no-ops on `.codex` installs. +const codexProjectHook = (skillDir) => `${skillDir}/skills/impeccable/scripts/hook.mjs`; const CURSOR_BEFORE_EDIT_SCRIPT = '.cursor/skills/impeccable/scripts/hook-before-edit.mjs'; const GITHUB_PROJECT_HOOK = '$(git rev-parse --show-toplevel)/.github/skills/impeccable/scripts/hook.mjs'; // Grok project hooks are relative to the git/workspace root. Claude tool names @@ -134,7 +140,11 @@ export function buildCodexPluginHooksManifest() { }; } -export function buildCodexHooksManifest() { +// `skillDir` is the install's own dot-directory (a provider's configDir), so the +// emitted command points at that install's payload. Defaults to `.codex` for the +// Codex provider, whose self-consistent bundle keeps the skill at `.codex/skills`. +export function buildCodexHooksManifest(skillDir = '.codex') { + const hookPath = codexProjectHook(skillDir); return { hooks: { PostToolUse: [ @@ -143,14 +153,14 @@ export function buildCodexHooksManifest() { hooks: [ { type: 'command', - command: guardedNode(CODEX_PROJECT_HOOK), + command: guardedNode(hookPath), timeout: TIMEOUT_SECONDS, statusMessage: STATUS_MESSAGE, }, ], }, ], - Stop: [stopEntry(guardedNode(CODEX_PROJECT_HOOK))], + Stop: [stopEntry(guardedNode(hookPath))], }, }; } @@ -222,12 +232,12 @@ export function buildGrokHooksManifest() { }; } -export function hooksJsonFor(provider) { +export function hooksJsonFor(provider, options = {}) { switch (provider) { case 'claude': return buildClaudeSettingsManifest(); case 'codex': - return buildCodexHooksManifest(); + return buildCodexHooksManifest(options.configDir || '.codex'); case 'cursor': return buildCursorHooksManifest(); case 'github': diff --git a/tests/hook-build.test.mjs b/tests/hook-build.test.mjs index e02641186..b13d65796 100644 --- a/tests/hook-build.test.mjs +++ b/tests/hook-build.test.mjs @@ -63,6 +63,9 @@ describe('hook manifest builders', () => { }); it('builds Codex project-local hooks for the real detector hook', () => { + // Default install dir is `.codex`: a `.codex`-directory install keeps the + // skill payload at `.codex/skills/...`, so the hook must point there (not at + // a hardcoded `.agents`, which no-ops on such installs). const manifest = buildCodexHooksManifest(); assert.equal(manifest.description, undefined); const group = manifest.hooks.PostToolUse[0]; @@ -72,7 +75,7 @@ describe('hook manifest builders', () => { assert.equal(handler.type, 'command'); assert.equal(handler.timeout, 5); assert.equal(handler.statusMessage, 'Checking UI changes'); - expectCommand(handler.command, '.agents/skills/impeccable/scripts/hook.mjs'); + expectCommand(handler.command, '.codex/skills/impeccable/scripts/hook.mjs'); assert.ok(!handler.command.includes('git rev-parse --show-toplevel')); assert.ok(!handler.command.includes('${PLUGIN_ROOT}')); assert.equal(manifest.hooks.SessionStart, undefined); @@ -81,7 +84,31 @@ describe('hook manifest builders', () => { // pass too. const stop = manifest.hooks.Stop[0].hooks[0]; assert.equal(stop.timeout, 30); - expectCommand(stop.command, '.agents/skills/impeccable/scripts/hook.mjs'); + expectCommand(stop.command, '.codex/skills/impeccable/scripts/hook.mjs'); + }); + + it('derives the Codex hook payload path from the install dir', () => { + // Each install dir gets a manifest pointing at its own skills payload: a + // `.codex`-directory install at `.codex/skills`, a `.agents` (Codex repo + // skills) install at `.agents/skills`. + const codexDir = buildCodexHooksManifest('.codex'); + expectCommand(codexDir.hooks.PostToolUse[0].hooks[0].command, '.codex/skills/impeccable/scripts/hook.mjs'); + expectCommand(codexDir.hooks.Stop[0].hooks[0].command, '.codex/skills/impeccable/scripts/hook.mjs'); + + const agentsDir = buildCodexHooksManifest('.agents'); + expectCommand(agentsDir.hooks.PostToolUse[0].hooks[0].command, '.agents/skills/impeccable/scripts/hook.mjs'); + expectCommand(agentsDir.hooks.Stop[0].hooks[0].command, '.agents/skills/impeccable/scripts/hook.mjs'); + assert.ok(!agentsDir.hooks.PostToolUse[0].hooks[0].command.includes('.codex/skills')); + + // hooksJsonFor threads the provider's configDir through to the builder. + expectCommand( + hooksJsonFor('codex', { configDir: '.agents' }).hooks.PostToolUse[0].hooks[0].command, + '.agents/skills/impeccable/scripts/hook.mjs', + ); + expectCommand( + hooksJsonFor('codex').hooks.PostToolUse[0].hooks[0].command, + '.codex/skills/impeccable/scripts/hook.mjs', + ); }); it('builds one Cursor pre-write blocking hook', () => { @@ -192,11 +219,24 @@ describe('generated hook artifacts in repo', () => { assert.ok(fs.existsSync(path.join(REPO_ROOT, '.cursor/skills/impeccable/scripts/detector/detect-antipatterns.mjs'))); }); - it('Codex project hooks reference hook.mjs in the .agents skill payload', () => { + it('Codex project hooks reference hook.mjs in the .codex skill payload', () => { + // The committed `.codex/hooks.json` is the distribution artifact for a + // `.codex`-directory install, whose skill payload lives at `.codex/skills/` + // (issue: it previously hardcoded `.agents/skills`, so the guarded hook + // no-opped on `.codex` installs). CLI installs that lay the skill down at + // `.agents/skills` rewrite the command to that path at install time. const manifest = readJson('.codex/hooks.json'); const handler = manifest.hooks.PostToolUse[0].hooks[0]; - expectCommand(handler.command, '.agents/skills/impeccable/scripts/hook.mjs'); + expectCommand(handler.command, '.codex/skills/impeccable/scripts/hook.mjs'); + assert.ok(!handler.command.includes('.agents/skills')); + + // The self-consistent Codex bundle ships the payload the manifest targets. + assert.ok(fs.existsSync(path.join(REPO_ROOT, 'dist/codex/.codex/skills/impeccable/SKILL.md'))); + assert.ok(fs.existsSync(path.join(REPO_ROOT, 'dist/codex/.codex/skills/impeccable/scripts/hook.mjs'))); + + // The repo also ships the Codex skill payload at `.agents/skills` (the + // layout CLI installs use, and where the rewritten command resolves). assert.ok(fs.existsSync(path.join(REPO_ROOT, '.agents/skills/impeccable/SKILL.md'))); assert.ok(fs.existsSync(path.join(REPO_ROOT, '.agents/skills/impeccable/scripts/hook.mjs'))); assert.ok(fs.existsSync(path.join(REPO_ROOT, '.agents/skills/impeccable/scripts/hook-lib.mjs'))); diff --git a/tests/skills-cli.test.js b/tests/skills-cli.test.js index e73db0976..556afe8d5 100644 --- a/tests/skills-cli.test.js +++ b/tests/skills-cli.test.js @@ -98,8 +98,11 @@ function createFakeUniversalBundle(root, providers = ['.claude', '.agents', '.cu } if (providers.includes('.agents')) { mkdirSync(join(bundleRoot, '.codex'), { recursive: true }); + // Mirror production: the Codex bundle's `.codex/hooks.json` targets its own + // `.codex/skills` payload. The CLI installs the skill at `.agents/skills`, so + // the installer must rewrite this command to `.agents/skills` (see below). writeFileSync(join(bundleRoot, '.codex', 'hooks.json'), JSON.stringify({ - hooks: { PostToolUse: [{ matcher: 'apply_patch', hooks: [{ type: 'command', command: 'node ".agents/skills/impeccable/scripts/hook.mjs"' }] }] }, + hooks: { PostToolUse: [{ matcher: 'apply_patch', hooks: [{ type: 'command', command: 'node ".codex/skills/impeccable/scripts/hook.mjs"' }] }] }, }, null, 2)); } return bundleRoot; @@ -675,6 +678,12 @@ describe('skills install/update: local universal bundle e2e', () => { expect(existsSync(join(tmp, '.claude', 'settings.local.json'))).toBe(true); expect(existsSync(join(tmp, '.cursor', 'hooks.json'))).toBe(true); expect(existsSync(join(tmp, '.codex', 'hooks.json'))).toBe(true); + // The CLI puts Codex's skill at `.agents/skills`, so the project-scope hook + // command must point there — not at the bundle's own `.codex/skills` path, + // which would resolve to a nonexistent file and silently no-op the hook. + const codexHooks = readFileSync(join(tmp, '.codex', 'hooks.json'), 'utf8'); + expect(codexHooks).toContain('.agents/skills/impeccable/scripts/hook.mjs'); + expect(codexHooks).not.toContain('.codex/skills/impeccable/scripts/hook.mjs'); rmSync(tmp, { recursive: true, force: true }); }, 15000);