From 66498ae9acec6430c5b41411cce6ed4b0aadb7f1 Mon Sep 17 00:00:00 2001 From: Jonghyeok Park Date: Sun, 8 Mar 2026 16:53:20 +0900 Subject: [PATCH] perf(hooks): use direct require() instead of spawning child process Invoke hook scripts directly via require() when they export a run(rawInput) function, eliminating one Node.js process spawn per hook invocation (~50-100ms). Includes path traversal guard, timeouts, error logging, PR review feedback, legacy hooks guard, normalized filePath, and restored findProjectRoot config detection with package manager support. --- scripts/hooks/post-edit-format.js | 23 +++--- scripts/hooks/quality-gate.js | 49 ++++++++++--- scripts/hooks/run-with-flags.js | 42 ++++++++++- scripts/lib/resolve-formatter.js | 106 ++++++++++++++++++++-------- tests/lib/resolve-formatter.test.js | 68 +++++++++++++++--- tests/run-all.js | 3 +- 6 files changed, 228 insertions(+), 63 deletions(-) diff --git a/scripts/hooks/post-edit-format.js b/scripts/hooks/post-edit-format.js index 89643830..7e098132 100644 --- a/scripts/hooks/post-edit-format.js +++ b/scripts/hooks/post-edit-format.js @@ -20,11 +20,7 @@ const { execFileSync } = require('child_process'); const path = require('path'); -const { - findProjectRoot, - detectFormatter, - resolveFormatterBin, -} = require('../lib/resolve-formatter'); +const { findProjectRoot, detectFormatter, resolveFormatterBin } = require('../lib/resolve-formatter'); const MAX_STDIN = 1024 * 1024; // 1MB limit @@ -42,23 +38,22 @@ function run(rawInput) { if (filePath && /\.(ts|tsx|js|jsx)$/.test(filePath)) { try { - const projectRoot = findProjectRoot(path.dirname(path.resolve(filePath))); + const resolvedFilePath = path.resolve(filePath); + const projectRoot = findProjectRoot(path.dirname(resolvedFilePath)); const formatter = detectFormatter(projectRoot); if (!formatter) return rawInput; const resolved = resolveFormatterBin(projectRoot, formatter); + if (!resolved) return rawInput; // Biome: `check --write` = format + lint in one pass // Prettier: `--write` = format only - const args = - formatter === 'biome' - ? [...resolved.prefix, 'check', '--write', filePath] - : [...resolved.prefix, '--write', filePath]; + const args = formatter === 'biome' ? [...resolved.prefix, 'check', '--write', resolvedFilePath] : [...resolved.prefix, '--write', resolvedFilePath]; execFileSync(resolved.bin, args, { cwd: projectRoot, stdio: ['pipe', 'pipe', 'pipe'], - timeout: 15000, + timeout: 15000 }); } catch { // Formatter not installed, file missing, or failed — non-blocking @@ -76,7 +71,7 @@ if (require.main === module) { let data = ''; process.stdin.setEncoding('utf8'); - process.stdin.on('data', (chunk) => { + process.stdin.on('data', chunk => { if (data.length < MAX_STDIN) { const remaining = MAX_STDIN - data.length; data += chunk.substring(0, remaining); @@ -84,8 +79,8 @@ if (require.main === module) { }); process.stdin.on('end', () => { - const result = run(data); - process.stdout.write(result); + data = run(data); + process.stdout.write(data); process.exit(0); }); } diff --git a/scripts/hooks/quality-gate.js b/scripts/hooks/quality-gate.js index aee94b27..19f35195 100755 --- a/scripts/hooks/quality-gate.js +++ b/scripts/hooks/quality-gate.js @@ -18,31 +18,50 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); -const { - findProjectRoot, - detectFormatter, - resolveFormatterBin, -} = require('../lib/resolve-formatter'); +const { findProjectRoot, detectFormatter, resolveFormatterBin } = require('../lib/resolve-formatter'); const MAX_STDIN = 1024 * 1024; +/** + * Execute a command synchronously, returning the spawnSync result. + * + * @param {string} command - Executable path or name + * @param {string[]} args - Arguments to pass + * @param {string} [cwd] - Working directory (defaults to process.cwd()) + * @returns {import('child_process').SpawnSyncReturns} + */ function exec(command, args, cwd = process.cwd()) { return spawnSync(command, args, { cwd, encoding: 'utf8', env: process.env, + timeout: 15000 }); } +/** + * Write a message to stderr for logging. + * + * @param {string} msg - Message to log + */ function log(msg) { process.stderr.write(`${msg}\n`); } +/** + * Run quality-gate checks for a single file based on its extension. + * Skips JS/TS files when Biome is configured (handled by post-edit-format). + * + * @param {string} filePath - Path to the edited file + */ function maybeRunQualityGate(filePath) { if (!filePath || !fs.existsSync(filePath)) { return; } + // Resolve to absolute path so projectRoot-relative comparisons work + filePath = path.resolve(filePath); + const ext = path.extname(filePath).toLowerCase(); const fix = String(process.env.ECC_QUALITY_GATE_FIX || '').toLowerCase() === 'true'; const strict = String(process.env.ECC_QUALITY_GATE_STRICT || '').toLowerCase() === 'true'; @@ -59,6 +78,7 @@ function maybeRunQualityGate(filePath) { // .json / .md — still need quality gate const resolved = resolveFormatterBin(projectRoot, 'biome'); + if (!resolved) return; const args = [...resolved.prefix, 'check', filePath]; if (fix) args.push('--write'); const result = exec(resolved.bin, args, projectRoot); @@ -70,6 +90,7 @@ function maybeRunQualityGate(filePath) { if (formatter === 'prettier') { const resolved = resolveFormatterBin(projectRoot, 'prettier'); + if (!resolved) return; const args = [...resolved.prefix, fix ? '--write' : '--check', filePath]; const result = exec(resolved.bin, args, projectRoot); if (result.status !== 0 && strict) { @@ -82,8 +103,20 @@ function maybeRunQualityGate(filePath) { return; } - if (ext === '.go' && fix) { - exec('gofmt', ['-w', filePath]); + if (ext === '.go') { + if (fix) { + const r = exec('gofmt', ['-w', filePath]); + if (r.status !== 0 && strict) { + log(`[QualityGate] gofmt failed for ${filePath}`); + } + } else if (strict) { + const r = exec('gofmt', ['-l', filePath]); + if (r.status !== 0) { + log(`[QualityGate] gofmt failed for ${filePath}`); + } else if (r.stdout && r.stdout.trim()) { + log(`[QualityGate] gofmt check failed for ${filePath}`); + } + } return; } @@ -119,7 +152,7 @@ function run(rawInput) { if (require.main === module) { let raw = ''; process.stdin.setEncoding('utf8'); - process.stdin.on('data', (chunk) => { + process.stdin.on('data', chunk => { if (raw.length < MAX_STDIN) { const remaining = MAX_STDIN - raw.length; raw += chunk.substring(0, remaining); diff --git a/scripts/hooks/run-with-flags.js b/scripts/hooks/run-with-flags.js index eb41564f..d69e8131 100755 --- a/scripts/hooks/run-with-flags.js +++ b/scripts/hooks/run-with-flags.js @@ -52,7 +52,15 @@ async function main() { } const pluginRoot = getPluginRoot(); - const scriptPath = path.join(pluginRoot, relScriptPath); + const resolvedRoot = path.resolve(pluginRoot); + const scriptPath = path.resolve(pluginRoot, relScriptPath); + + // Prevent path traversal outside the plugin root + if (!scriptPath.startsWith(resolvedRoot + path.sep)) { + process.stderr.write(`[Hook] Path traversal rejected for ${hookId}: ${scriptPath}\n`); + process.stdout.write(raw); + process.exit(0); + } if (!fs.existsSync(scriptPath)) { process.stderr.write(`[Hook] Script not found for ${hookId}: ${scriptPath}\n`); @@ -60,11 +68,43 @@ async function main() { process.exit(0); } + // Prefer direct require() when the hook exports a run(rawInput) function. + // This eliminates one Node.js process spawn (~50-100ms savings per hook). + // + // SAFETY: Only require() hooks that export run(). Legacy hooks execute + // side effects at module scope (stdin listeners, process.exit, main() calls) + // which would interfere with the parent process or cause double execution. + let hookModule; + const src = fs.readFileSync(scriptPath, 'utf8'); + const hasRunExport = /\bmodule\.exports\b/.test(src) && /\brun\b/.test(src); + + if (hasRunExport) { + try { + hookModule = require(scriptPath); + } catch (requireErr) { + process.stderr.write(`[Hook] require() failed for ${hookId}: ${requireErr.message}\n`); + // Fall through to legacy spawnSync path + } + } + + if (hookModule && typeof hookModule.run === 'function') { + try { + const output = hookModule.run(raw); + if (output != null) process.stdout.write(output); + } catch (runErr) { + process.stderr.write(`[Hook] run() error for ${hookId}: ${runErr.message}\n`); + process.stdout.write(raw); + } + process.exit(0); + } + + // Legacy path: spawn a child Node process for hooks without run() export const result = spawnSync('node', [scriptPath], { input: raw, encoding: 'utf8', env: process.env, cwd: process.cwd(), + timeout: 30000 }); if (result.stdout) process.stdout.write(result.stdout); diff --git a/scripts/lib/resolve-formatter.js b/scripts/lib/resolve-formatter.js index 727a0734..f940a1fa 100644 --- a/scripts/lib/resolve-formatter.js +++ b/scripts/lib/resolve-formatter.js @@ -18,9 +18,29 @@ const binCache = new Map(); // ── Public helpers ────────────────────────────────────────────────── +// Markers that indicate a project root (formatter configs included so +// repos without package.json are still detected correctly). +const PROJECT_ROOT_MARKERS = [ + 'package.json', + 'biome.json', + 'biome.jsonc', + '.prettierrc', + '.prettierrc.json', + '.prettierrc.js', + '.prettierrc.cjs', + '.prettierrc.mjs', + '.prettierrc.yml', + '.prettierrc.yaml', + '.prettierrc.toml', + 'prettier.config.js', + 'prettier.config.cjs', + 'prettier.config.mjs' +]; + /** - * Walk up from `startDir` until a directory containing package.json is found. - * Returns `startDir` as fallback when no package.json exists above it. + * Walk up from `startDir` until a directory containing a known project + * root marker (package.json or formatter config) is found. + * Returns `startDir` as fallback when no marker exists above it. * * @param {string} startDir - Absolute directory path to start from * @returns {string} Absolute path to the project root @@ -30,9 +50,11 @@ function findProjectRoot(startDir) { let dir = startDir; while (dir !== path.dirname(dir)) { - if (fs.existsSync(path.join(dir, 'package.json'))) { - projectRootCache.set(startDir, dir); - return dir; + for (const marker of PROJECT_ROOT_MARKERS) { + if (fs.existsSync(path.join(dir, marker))) { + projectRootCache.set(startDir, dir); + return dir; + } } dir = path.dirname(dir); } @@ -59,6 +81,20 @@ function detectFormatter(projectRoot) { } } + // Check package.json "prettier" key before config files + try { + const pkgPath = path.join(projectRoot, 'package.json'); + if (fs.existsSync(pkgPath)) { + const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); + if (pkg.prettier != null) { + formatterCache.set(projectRoot, 'prettier'); + return 'prettier'; + } + } + } catch { + // Malformed package.json — continue to file-based detection + } + const prettierConfigs = [ '.prettierrc', '.prettierrc.json', @@ -70,7 +106,7 @@ function detectFormatter(projectRoot) { '.prettierrc.toml', 'prettier.config.js', 'prettier.config.cjs', - 'prettier.config.mjs', + 'prettier.config.mjs' ]; for (const cfg of prettierConfigs) { if (fs.existsSync(path.join(projectRoot, cfg))) { @@ -83,14 +119,35 @@ function detectFormatter(projectRoot) { return null; } +/** + * Resolve the runner binary and prefix args for the configured package + * manager (respects CLAUDE_PACKAGE_MANAGER env and project config). + * + * @param {string} projectRoot - Absolute path to the project root + * @returns {{ bin: string, prefix: string[] }} + */ +// Windows .cmd shim mapping for cross-platform safety +const WIN_CMD_SHIMS = { npx: 'npx.cmd', pnpm: 'pnpm.cmd', yarn: 'yarn.cmd', bunx: 'bunx.cmd' }; + +function getRunnerFromPackageManager(projectRoot) { + const isWin = process.platform === 'win32'; + const { getPackageManager } = require('./package-manager'); + const pm = getPackageManager({ projectDir: projectRoot }); + const execCmd = pm?.config?.execCmd || 'npx'; + const [rawBin = 'npx', ...prefix] = execCmd.split(/\s+/).filter(Boolean); + const bin = isWin ? WIN_CMD_SHIMS[rawBin] || rawBin : rawBin; + return { bin, prefix }; +} + /** * Resolve the formatter binary, preferring the local node_modules/.bin - * installation over npx to avoid package-resolution overhead. + * installation over the package manager exec command to avoid + * package-resolution overhead. * * @param {string} projectRoot - Absolute path to the project root * @param {'biome' | 'prettier'} formatter - Detected formatter name - * @returns {{ bin: string, prefix: string[] }} - * `bin` – executable path (absolute local path or npx/npx.cmd) + * @returns {{ bin: string, prefix: string[] } | null} + * `bin` – executable path (absolute local path or runner binary) * `prefix` – extra args to prepend (e.g. ['@biomejs/biome'] when using npx) */ function resolveFormatterBin(projectRoot, formatter) { @@ -98,45 +155,36 @@ function resolveFormatterBin(projectRoot, formatter) { if (binCache.has(cacheKey)) return binCache.get(cacheKey); const isWin = process.platform === 'win32'; - const npxBin = isWin ? 'npx.cmd' : 'npx'; if (formatter === 'biome') { - const localBin = path.join( - projectRoot, - 'node_modules', - '.bin', - isWin ? 'biome.cmd' : 'biome', - ); + const localBin = path.join(projectRoot, 'node_modules', '.bin', isWin ? 'biome.cmd' : 'biome'); if (fs.existsSync(localBin)) { const result = { bin: localBin, prefix: [] }; binCache.set(cacheKey, result); return result; } - const result = { bin: npxBin, prefix: ['@biomejs/biome'] }; + const runner = getRunnerFromPackageManager(projectRoot); + const result = { bin: runner.bin, prefix: [...runner.prefix, '@biomejs/biome'] }; binCache.set(cacheKey, result); return result; } if (formatter === 'prettier') { - const localBin = path.join( - projectRoot, - 'node_modules', - '.bin', - isWin ? 'prettier.cmd' : 'prettier', - ); + const localBin = path.join(projectRoot, 'node_modules', '.bin', isWin ? 'prettier.cmd' : 'prettier'); if (fs.existsSync(localBin)) { const result = { bin: localBin, prefix: [] }; binCache.set(cacheKey, result); return result; } - const result = { bin: npxBin, prefix: ['prettier'] }; + const runner = getRunnerFromPackageManager(projectRoot); + const result = { bin: runner.bin, prefix: [...runner.prefix, 'prettier'] }; binCache.set(cacheKey, result); return result; } - const result = { bin: npxBin, prefix: [] }; - binCache.set(cacheKey, result); - return result; + // Unknown formatter — return null so callers can handle gracefully + binCache.set(cacheKey, null); + return null; } /** @@ -152,5 +200,5 @@ module.exports = { findProjectRoot, detectFormatter, resolveFormatterBin, - clearCaches, -}; \ No newline at end of file + clearCaches +}; diff --git a/tests/lib/resolve-formatter.test.js b/tests/lib/resolve-formatter.test.js index 92741f84..c02bb60d 100644 --- a/tests/lib/resolve-formatter.test.js +++ b/tests/lib/resolve-formatter.test.js @@ -9,14 +9,15 @@ const path = require('path'); const fs = require('fs'); const os = require('os'); -const { - findProjectRoot, - detectFormatter, - resolveFormatterBin, - clearCaches, -} = require('../../scripts/lib/resolve-formatter'); +const { findProjectRoot, detectFormatter, resolveFormatterBin, clearCaches } = require('../../scripts/lib/resolve-formatter'); -// Test helper +/** + * Run a single test case, printing pass/fail. + * + * @param {string} name - Test description + * @param {() => void} fn - Test body (throws on failure) + * @returns {boolean} Whether the test passed + */ function test(name, fn) { try { fn(); @@ -29,8 +30,32 @@ function test(name, fn) { } } +/** Track all created tmp dirs for cleanup */ +const tmpDirs = []; + +/** + * Create a temporary directory and track it for cleanup. + * + * @returns {string} Absolute path to the new temp directory + */ function makeTmpDir() { - return fs.mkdtempSync(path.join(os.tmpdir(), 'resolve-fmt-')); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'resolve-fmt-')); + tmpDirs.push(dir); + return dir; +} + +/** + * Remove all tracked temporary directories. + */ +function cleanupTmpDirs() { + for (const dir of tmpDirs) { + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { + // Best-effort cleanup + } + } + tmpDirs.length = 0; } function runTests() { @@ -103,6 +128,18 @@ function runTests() { assert.strictEqual(detectFormatter(root), 'prettier'); }); + run('detectFormatter: detects prettier key in package.json', () => { + const root = makeTmpDir(); + fs.writeFileSync(path.join(root, 'package.json'), JSON.stringify({ name: 'test', prettier: { singleQuote: true } })); + assert.strictEqual(detectFormatter(root), 'prettier'); + }); + + run('detectFormatter: ignores package.json without prettier key', () => { + const root = makeTmpDir(); + fs.writeFileSync(path.join(root, 'package.json'), JSON.stringify({ name: 'test' })); + assert.strictEqual(detectFormatter(root), null); + }); + run('detectFormatter: biome takes priority over prettier', () => { const root = makeTmpDir(); fs.writeFileSync(path.join(root, 'biome.json'), '{}'); @@ -157,6 +194,12 @@ function runTests() { assert.deepStrictEqual(result.prefix, ['prettier']); }); + run('resolveFormatterBin: returns null for unknown formatter', () => { + const root = makeTmpDir(); + const result = resolveFormatterBin(root, 'unknown'); + assert.strictEqual(result, null); + }); + run('resolveFormatterBin: caches resolved binary', () => { const root = makeTmpDir(); const binDir = path.join(root, 'node_modules', '.bin'); @@ -189,9 +232,14 @@ function runTests() { assert.strictEqual(detectFormatter(root), null); }); - // ── Summary ─────────────────────────────────────────────────── + // ── Summary & Cleanup ───────────────────────────────────────── - console.log(`\n ${passed} passed, ${failed} failed\n`); + cleanupTmpDirs(); + + console.log('\n=== Test Results ==='); + console.log(`Passed: ${passed}`); + console.log(`Failed: ${failed}`); + console.log(`Total: ${passed + failed}`); process.exit(failed > 0 ? 1 : 0); } diff --git a/tests/run-all.js b/tests/run-all.js index 54d8c212..41a6c7f7 100644 --- a/tests/run-all.js +++ b/tests/run-all.js @@ -16,6 +16,7 @@ const testFiles = [ 'lib/session-manager.test.js', 'lib/session-aliases.test.js', 'lib/project-detect.test.js', + 'lib/resolve-formatter.test.js', 'hooks/hooks.test.js', 'hooks/evaluate-session.test.js', 'hooks/suggest-compact.test.js', @@ -27,7 +28,7 @@ const testFiles = [ ]; const BOX_W = 58; // inner width between ║ delimiters -const boxLine = (s) => `║${s.padEnd(BOX_W)}║`; +const boxLine = s => `║${s.padEnd(BOX_W)}║`; console.log('╔' + '═'.repeat(BOX_W) + '╗'); console.log(boxLine(' Everything Claude Code - Test Suite'));