mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-15 15:46:30 +03:00
Fix Codex hook path so .codex-directory installs run the detector
The committed .codex/hooks.json hardcoded .agents/skills/impeccable/scripts/
hook.mjs. On a .codex-directory install the skill payload lives at .codex/
skills/..., so the guarded command ([ ! -f X ] || node X) found no file and
silently no-opped, leaving the design detector dead for those users.
Derive the hook payload path from the emitting provider's own configDir rather
than hardcoding .agents:
- buildCodexHooksManifest(skillDir) now builds `${skillDir}/skills/impeccable/
scripts/hook.mjs`; hooksJsonFor threads each provider's configDir through. The
Codex provider (configDir .codex) emits .codex/skills; the root sync and the
self-consistent dist/codex bundle both point at their own payload.
- CLI installer: project-scope hook rewriting now derives the provider's own
project-relative path instead of preserving the bundle token. The Codex bundle
ships a .codex/skills command, but the CLI lays the skill at .agents/skills, so
the installed .codex/hooks.json is rewritten to .agents/skills (Claude keeps
its ${CLAUDE_PROJECT_DIR} token; global installs keep the absolute rewrite).
Per-provider hook payload path after the fix:
Emission hook path
dist/codex/.codex/hooks.json .codex/skills/impeccable/scripts/hook.mjs
root .codex/hooks.json (build sync) .codex/skills/impeccable/scripts/hook.mjs
CLI .agents (codex) project install .agents/skills/impeccable/scripts/hook.mjs
CLI .agents (codex) global install <home>/.agents/skills/.../hook.mjs (abs)
.claude / .cursor unchanged
Tests: extended hook-build (codex-dir -> .codex/skills, agents-dir -> .agents/
skills) and skills-cli (bundle ships .codex/skills, install rewrites to .agents/
skills). Regenerated tracked .codex/hooks.json via build:release.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
bb57be4243
commit
bcf354cd0c
+2
-2
@@ -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"
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
+1
-1
@@ -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);
|
||||
|
||||
@@ -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');
|
||||
|
||||
@@ -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':
|
||||
|
||||
@@ -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')));
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user