Skip to content

perf(hooks): optimize formatter hooks(x52 faster) — local binary, merged invocations, direct require() #359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pythonstrup wants to merge 6 commits into
base: main
Choose a base branch
from
+4,437 −3,243
Open

perf(hooks): optimize formatter hooks(x52 faster) — local binary, merged invocations, direct require() #359

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e66bddb
feat(hooks): add shared resolve-formatter utility with caching
pythonstrup Mar 8, 2026
999c53d
perf(hooks): eliminate npx overhead and merge biome invocations
pythonstrup Mar 8, 2026
5137ad5
perf(hooks): use direct require() instead of spawning child process
pythonstrup Mar 8, 2026
34cd7b7
fix(hooks): add path traversal guard, timeouts, and error logging
pythonstrup Mar 8, 2026
3cc66de
fix(hooks): address PR review feedback for biome optimization
pythonstrup Mar 8, 2026
3abd3bf
fix(hooks): guard require() for legacy hooks, normalize filePath, fix…
pythonstrup Mar 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 54 additions & 69 deletions scripts/hooks/post-edit-format.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,88 +7,53 @@
* Runs after Edit tool use. If the edited file is a JS/TS file,
* auto-detects the project formatter (Biome or Prettier) by looking
* for config files, then formats accordingly.
*
* For Biome, uses `check --write` (format + lint in one pass) to
* avoid a redundant second invocation from quality-gate.js.
*
* Prefers the local node_modules/.bin binary over npx to skip
* package-resolution overhead (~200-500ms savings per invocation).
*
* Fails silently if no formatter is found or installed.
*/

const { execFileSync } = require('child_process');
const fs = require('fs');
const path = require('path');

const MAX_STDIN = 1024 * 1024; // 1MB limit
let data = '';
process.stdin.setEncoding('utf8');

process.stdin.on('data', chunk => {
if (data.length < MAX_STDIN) {
const remaining = MAX_STDIN - data.length;
data += chunk.substring(0, remaining);
}
});

function findProjectRoot(startDir) {
let dir = startDir;
while (dir !== path.dirname(dir)) {
if (fs.existsSync(path.join(dir, 'package.json'))) return dir;
dir = path.dirname(dir);
}
return startDir;
}

function detectFormatter(projectRoot) {
const biomeConfigs = ['biome.json', 'biome.jsonc'];
for (const cfg of biomeConfigs) {
if (fs.existsSync(path.join(projectRoot, cfg))) return 'biome';
}

const prettierConfigs = [
'.prettierrc',
'.prettierrc.json',
'.prettierrc.js',
'.prettierrc.cjs',
'.prettierrc.mjs',
'.prettierrc.yml',
'.prettierrc.yaml',
'.prettierrc.toml',
'prettier.config.js',
'prettier.config.cjs',
'prettier.config.mjs',
];
for (const cfg of prettierConfigs) {
if (fs.existsSync(path.join(projectRoot, cfg))) return 'prettier';
}

return null;
}
const { findProjectRoot, detectFormatter, resolveFormatterBin } = require('../lib/resolve-formatter');

function getFormatterCommand(formatter, filePath) {
const npxBin = process.platform === 'win32' ? 'npx.cmd' : 'npx';
if (formatter === 'biome') {
return { bin: npxBin, args: ['@biomejs/biome', 'format', '--write', filePath] };
}
if (formatter === 'prettier') {
return { bin: npxBin, args: ['prettier', '--write', filePath] };
}
return null;
}
const MAX_STDIN = 1024 * 1024; // 1MB limit

process.stdin.on('end', () => {
/**
* Core logic — exported so run-with-flags.js can call directly
* without spawning a child process.
*
* @param {string} rawInput - Raw JSON string from stdin
* @returns {string} The original input (pass-through)
*/
function run(rawInput) {
try {
const input = JSON.parse(data);
const input = JSON.parse(rawInput);
const filePath = input.tool_input?.file_path;

if (filePath && /\.(ts|tsx|js|jsx)$/.test(filePath)) {
try {
const projectRoot = findProjectRoot(path.dirname(path.resolve(filePath)));
const formatter = detectFormatter(projectRoot);
const cmd = getFormatterCommand(formatter, filePath);
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];

if (cmd) {
execFileSync(cmd.bin, cmd.args, {
cwd: projectRoot,
stdio: ['pipe', 'pipe', 'pipe'],
timeout: 15000
});
}
execFileSync(resolved.bin, args, {
cwd: projectRoot,
stdio: ['pipe', 'pipe', 'pipe'],
timeout: 15000
});
} catch {
// Formatter not installed, file missing, or failed — non-blocking
}
Expand All @@ -97,6 +62,26 @@ process.stdin.on('end', () => {
// Invalid input — pass through
}

process.stdout.write(data);
process.exit(0);
});
return rawInput;
}

// ── stdin entry point (backwards-compatible) ────────────────────
if (require.main === module) {
let data = '';
process.stdin.setEncoding('utf8');

process.stdin.on('data', chunk => {
if (data.length < MAX_STDIN) {
const remaining = MAX_STDIN - data.length;
data += chunk.substring(0, remaining);
}
});

process.stdin.on('end', () => {
const result = run(data);
process.stdout.write(result);
process.exit(0);
});
}

module.exports = { run };
126 changes: 97 additions & 29 deletions scripts/hooks/quality-gate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
* Runs lightweight quality checks after file edits.
* - Targets one file when file_path is provided
* - Falls back to no-op when language/tooling is unavailable
*
* For JS/TS files with Biome, this hook is skipped because
* post-edit-format.js already runs `biome check --write`.
* This hook still handles .json/.md files for Biome, and all
* Prettier / Go / Python checks.
*/

'use strict';
Expand All @@ -13,86 +18,149 @@ const fs = require('fs');
const path = require('path');
const { spawnSync } = require('child_process');

const { findProjectRoot, detectFormatter, resolveFormatterBin } = require('../lib/resolve-formatter');

const MAX_STDIN = 1024 * 1024;
let raw = '';

function run(command, args, cwd = process.cwd()) {
/**
* 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<string>}
*/
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';

if (['.ts', '.tsx', '.js', '.jsx', '.json', '.md'].includes(ext)) {
// Prefer biome if present
if (fs.existsSync(path.join(process.cwd(), 'biome.json')) || fs.existsSync(path.join(process.cwd(), 'biome.jsonc'))) {
const args = ['biome', 'check', filePath];
const projectRoot = findProjectRoot(path.dirname(path.resolve(filePath)));
const formatter = detectFormatter(projectRoot);

if (formatter === 'biome') {
// JS/TS already handled by post-edit-format via `biome check --write`
if (['.ts', '.tsx', '.js', '.jsx'].includes(ext)) {
return;
}

// .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 = run('npx', args);
const result = exec(resolved.bin, args, projectRoot);
if (result.status !== 0 && strict) {
log(`[QualityGate] Biome check failed for ${filePath}`);
}
return;
}

// Fallback to prettier when installed
const prettierArgs = ['prettier', '--check', filePath];
if (fix) {
prettierArgs[1] = '--write';
}
const prettier = run('npx', prettierArgs);
if (prettier.status !== 0 && strict) {
log(`[QualityGate] Prettier check failed for ${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) {
log(`[QualityGate] Prettier check failed for ${filePath}`);
}
return;
}

// No formatter configured — skip
return;
}

if (ext === '.go' && fix) {
run('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.stdout && r.stdout.trim()) {
log(`[QualityGate] gofmt check failed for ${filePath}`);
}
Comment on lines +112 to +118
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Treat gofmt -l execution errors as failures in strict mode.

Syntax errors and missing gofmt binaries return non-zero with empty stdout. This branch only logs when stdout contains filenames, so those failures are currently silent.

💡 Proposed fix 
     } else if (strict) {
       const r = exec('gofmt', ['-l', filePath]);
-      if (r.stdout && r.stdout.trim()) {
+      if (r.status !== 0 || (r.stdout && r.stdout.trim())) {
         log(`[QualityGate] gofmt check failed for ${filePath}`);
       }
     }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@scripts/hooks/quality-gate.js` around lines 112 - 116, The gofmt strict
branch currently only flags failures when r.stdout contains filenames, so
execution errors (non-zero exit, missing binary, or syntax errors) with empty
stdout are silent; update the handling around the exec('gofmt', ['-l',
filePath]) call (variable r) to treat non-zero exit codes or presence of
r.stderr/r.error as failures in strict mode by logging a failure for filePath
and include r.status (or r.code) and r.stderr in the log message (and still
treat any non-empty r.stdout as a failure as today).

}
return;
}

if (ext === '.py') {
const args = ['format'];
if (!fix) args.push('--check');
args.push(filePath);
const r = run('ruff', args);
const r = exec('ruff', args);
if (r.status !== 0 && strict) {
log(`[QualityGate] Ruff check failed for ${filePath}`);
}
}
}

process.stdin.setEncoding('utf8');
process.stdin.on('data', chunk => {
if (raw.length < MAX_STDIN) {
const remaining = MAX_STDIN - raw.length;
raw += chunk.substring(0, remaining);
}
});

process.stdin.on('end', () => {
/**
* Core logic — exported so run-with-flags.js can call directly.
*
* @param {string} rawInput - Raw JSON string from stdin
* @returns {string} The original input (pass-through)
*/
function run(rawInput) {
try {
const input = JSON.parse(raw);
const input = JSON.parse(rawInput);
const filePath = String(input.tool_input?.file_path || '');
maybeRunQualityGate(filePath);
} catch {
// Ignore parse errors.
}
return rawInput;
}

// ── stdin entry point (backwards-compatible) ────────────────────
if (require.main === module) {
let raw = '';
process.stdin.setEncoding('utf8');
process.stdin.on('data', chunk => {
if (raw.length < MAX_STDIN) {
const remaining = MAX_STDIN - raw.length;
raw += chunk.substring(0, remaining);
}
});

process.stdin.on('end', () => {
const result = run(raw);
process.stdout.write(result);
});
}

process.stdout.write(raw);
});
module.exports = { run };
34 changes: 33 additions & 1 deletion scripts/hooks/run-with-flags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,51 @@
}

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`);
process.stdout.write(raw);
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).
// NOTE: require() and run() errors are separated so that a run() failure
// does not fall through to the legacy spawnSync path (which would double-execute).
let hookModule;
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);

Check warning on line 86 in scripts/hooks/run-with-flags.js

View workflow job for this annotation

GitHub Actions / Lint 

Expected '!==' and instead saw '!='
} catch (runErr) {
process.stderr.write(`[Hook] run() error for ${hookId}: ${runErr.message}\n`);
}
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);
Expand Down
Loading
Loading