perf(hooks): optimize formatter hooks(x52 faster) — local binary, merged invocations, direct require()#359
Open
pythonstrup wants to merge 6 commits intoaffaan-m:mainfrom
Open
Conversation
Extract project-root discovery, formatter detection, and binary resolution into a reusable module. Caches results per-process to avoid redundant filesystem lookups on every Edit hook invocation. This is the foundation for eliminating npx overhead in format hooks.
- Use local node_modules/.bin/biome binary instead of npx (~200-500ms savings) - Change post-edit-format from `biome format --write` to `biome check --write` (format + lint in one pass) - Skip redundant biome check in quality-gate for JS/TS files already handled by post-edit-format - Fix quality-gate to use findProjectRoot instead of process.cwd() - Export run() function from both hooks for direct invocation - Update tests to match shared resolve-formatter module usage
When a hook script exports a run(rawInput) function, invoke it
directly via require() instead of spawnSync('node', ...). This
eliminates one Node.js process spawn per hook invocation (~50-100ms).
Hooks without a run() export fall back to the legacy spawnSync path,
maintaining full backwards compatibility.
Address code review findings: - Add path containment check before require() in run-with-flags.js to prevent path traversal outside plugin root - Add timeout (15s) to quality-gate.js exec() wrapper - Add timeout (30s) to run-with-flags.js legacy spawnSync path - Log require() errors to stderr instead of silently swallowing
Contributor
📝 WalkthroughWalkthroughCentralizes formatter detection and binary resolution into a new utility and refactors hook scripts to export a run(rawInput) API; the runner tries to require and invoke run(), falls back to spawning the legacy process, and adds path guards and per-process caching for formatter resolution. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as "run-with-flags.js"
participant Hook as "hook script (post-edit-format / quality-gate)"
participant Resolver as "resolve-formatter.js"
participant FS as "Filesystem"
participant Formatter as "Formatter binary"
Runner->>Hook: require(scriptPath).run(rawInput)
Hook->>Resolver: findProjectRoot(startDir)
Resolver->>FS: walk parent dirs for package.json
FS-->>Resolver: package.json path / not found
Resolver-->>Hook: projectRoot
Hook->>Resolver: detectFormatter(projectRoot)
Resolver->>FS: check biome/prettier config files
FS-->>Resolver: config presence (biome|prettier|null)
Resolver-->>Hook: formatter type
Hook->>Resolver: resolveFormatterBin(projectRoot, formatter)
Resolver->>FS: look for node_modules/.bin/<bin>
FS-->>Resolver: local bin path / none
Resolver-->>Hook: resolved.bin (local or npx prefix)
Hook->>Formatter: execFileSync(resolved.bin, args)
Formatter-->>Hook: exit code / output
Hook-->>Runner: return/write rawInput (or log error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/hooks/quality-gate.js">
<violation number="1" location="scripts/hooks/quality-gate.js:59">
P2: Relative `filePath` will not resolve correctly when `projectRoot` differs from `process.cwd()`. The exec call uses `projectRoot` as cwd, but `filePath` may be relative to `process.cwd()`. Resolve it to an absolute path before passing it to the formatter.
The same issue applies to the Prettier branch below.</violation>
</file>
<file name="scripts/hooks/run-with-flags.js">
<violation number="1" location="scripts/hooks/run-with-flags.js:73">
P1: The single try/catch wraps both `require()` and `run()`, so a `run()` error is misreported as "require() failed" and silently falls through to the legacy spawnSync path, executing the hook a second time.
Separate the two so that only a `require()` failure falls through to the legacy path, while a `run()` failure is surfaced without double-execution.</violation>
</file>
<file name="scripts/lib/resolve-formatter.js">
<violation number="1" location="scripts/lib/resolve-formatter.js:62">
P2: Missing detection of `"prettier"` key in `package.json`. This is a standard and commonly-used Prettier configuration method. Projects relying solely on this key won't have Prettier detected, silently skipping formatting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/hooks/run-with-flags.js (1)
71-83: Consider handling empty string output fromrun().Line 77's
if (output)check will skip writing whenrun()returns an empty string"", which is falsy in JavaScript. If a hook legitimately needs to return empty output, this would silently drop it. Use explicitoutput != nullif empty strings should be written.♻️ Explicit null check
if (typeof hookModule.run === 'function') { const output = hookModule.run(raw); - if (output) process.stdout.write(output); + if (output != null) process.stdout.write(output); process.exit(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/run-with-flags.js` around lines 71 - 83, The current require() path drops empty-string outputs because the check uses if (output), which treats "" as falsy; update the condition in the try block around hookModule.run(raw) so it writes any non-null/undefined result (e.g., use an explicit null/undefined check like output != null or output !== undefined) before calling process.stdout.write and process.exit, referencing the hookModule.run, raw, process.stdout.write, and process.exit locations in the snippet.tests/lib/resolve-formatter.test.js (1)
32-34: Consider cleaning up temporary directories after tests.The test creates temporary directories via
makeTmpDir()but never cleans them up. Whileos.tmpdir()directories may be cleared periodically by the OS, explicitly cleaning up in anafterEachor at test completion would prevent accumulation during repeated test runs.🧹 Optional cleanup helper
function makeTmpDir() { return fs.mkdtempSync(path.join(os.tmpdir(), 'resolve-fmt-')); } +const tmpDirs = []; + +function makeTmpDirTracked() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'resolve-fmt-')); + tmpDirs.push(dir); + return dir; +} + +function cleanupTmpDirs() { + for (const dir of tmpDirs) { + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { /* ignore */ } + } + tmpDirs.length = 0; +}Then call
cleanupTmpDirs()at the end ofrunTests().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lib/resolve-formatter.test.js` around lines 32 - 34, Tests create temp dirs via makeTmpDir() but never remove them; implement a small cleanup mechanism: track created directories in makeTmpDir (e.g., push each returned path into an array) and add a cleanupTmpDirs function that iterates the array and removes each directory (fs.rmSync with recursive: true or equivalent). Call cleanupTmpDirs from an appropriate teardown hook (e.g., afterEach or afterAll) or at the end of runTests() so temporary directories are removed after tests complete; update references to makeTmpDir, cleanupTmpDirs, and runTests accordingly.scripts/hooks/quality-gate.js (1)
82-95: Go format errors are silently ignored.For Go files,
gofmterrors are not logged even whenstrictmode is enabled. This differs from Python/Ruff which logs failures in strict mode.♻️ Consistent strict mode handling for Go
if (ext === '.go' && fix) { - exec('gofmt', ['-w', filePath]); + const r = exec('gofmt', ['-w', filePath]); + if (r.status !== 0 && strict) { + log(`[QualityGate] gofmt failed for ${filePath}`); + } return; }🤖 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 82 - 95, The Go branch currently calls exec('gofmt', ['-w', filePath]) and returns without checking the result; change it to capture the exec result (e.g., const r = exec('gofmt', ['-w', filePath])) and if r.status !== 0 and strict is true, log a descriptive message (similar to the Python branch) including filePath and any r.stderr/r.stdout to surface the formatting error; keep the existing behavior when fix is false (no-op) but ensure strict-mode failures are reported for the 'gofmt' invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/hooks/quality-gate.js`:
- Around line 51-55: The hook currently sets async execution for the
quality-gate hook while it intentionally skips JS/TS when formatter === 'biome'
(the ext check for ['.ts', '.tsx', '.js', '.jsx']), which can create a race with
the synchronous post-edit-format hook; to fix, remove the async flag from the
quality-gate hook (making it run synchronously) so its skip behavior reliably
occurs before post-edit-format runs, or if you prefer not to change execution
semantics, add a concise comment inside the block referencing the intentional
skip and the dependency on post-edit-format (mentioning post-edit-format and the
'biome' check) explaining the required ordering guarantee from the hook runner.
In `@scripts/lib/resolve-formatter.js`:
- Around line 96-140: The function resolveFormatterBin currently falls back to {
bin: npxBin, prefix: [] } for unknown formatter values which would invoke npx
with no package; instead validate the formatter at the end of
resolveFormatterBin (function name) and handle unknown values defensively by
caching and returning null (e.g., binCache.set(cacheKey, null); return null) so
callers can detect and skip/raise a clear error; ensure you reference the same
cacheKey and not return an executable object, and update callers to check for
null (binCache, projectRoot, formatter are the unique symbols to locate the
logic).
---
Nitpick comments:
In `@scripts/hooks/quality-gate.js`:
- Around line 82-95: The Go branch currently calls exec('gofmt', ['-w',
filePath]) and returns without checking the result; change it to capture the
exec result (e.g., const r = exec('gofmt', ['-w', filePath])) and if r.status
!== 0 and strict is true, log a descriptive message (similar to the Python
branch) including filePath and any r.stderr/r.stdout to surface the formatting
error; keep the existing behavior when fix is false (no-op) but ensure
strict-mode failures are reported for the 'gofmt' invocation.
In `@scripts/hooks/run-with-flags.js`:
- Around line 71-83: The current require() path drops empty-string outputs
because the check uses if (output), which treats "" as falsy; update the
condition in the try block around hookModule.run(raw) so it writes any
non-null/undefined result (e.g., use an explicit null/undefined check like
output != null or output !== undefined) before calling process.stdout.write and
process.exit, referencing the hookModule.run, raw, process.stdout.write, and
process.exit locations in the snippet.
In `@tests/lib/resolve-formatter.test.js`:
- Around line 32-34: Tests create temp dirs via makeTmpDir() but never remove
them; implement a small cleanup mechanism: track created directories in
makeTmpDir (e.g., push each returned path into an array) and add a
cleanupTmpDirs function that iterates the array and removes each directory
(fs.rmSync with recursive: true or equivalent). Call cleanupTmpDirs from an
appropriate teardown hook (e.g., afterEach or afterAll) or at the end of
runTests() so temporary directories are removed after tests complete; update
references to makeTmpDir, cleanupTmpDirs, and runTests accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21e346ed-eb36-4cc4-90bc-00424bea608e
📒 Files selected for processing (6)
scripts/hooks/post-edit-format.jsscripts/hooks/quality-gate.jsscripts/hooks/run-with-flags.jsscripts/lib/resolve-formatter.jstests/hooks/hooks.test.jstests/lib/resolve-formatter.test.js
Address 8 PR review issues from cubic-dev-ai and coderabbitai: - Separate require()/run() try/catch in run-with-flags to prevent double-execution on run() errors (affaan-m#1) - Resolve filePath to absolute in quality-gate (#2) - Detect prettier key in package.json (affaan-m#3) - Use `output != null` instead of truthy check (affaan-m#4) - Add gofmt strict mode logging (affaan-m#5) - Return null for unknown formatter in resolveFormatterBin (#6) - Track and cleanup temp dirs in tests (#7) - Add JSDoc to exported functions (#8)
There was a problem hiding this comment.
3 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/hooks/run-with-flags.js">
<violation number="1" location="scripts/hooks/run-with-flags.js:87">
P1: When `run()` throws, the original input `raw` is never written to stdout and the process exits with code 0. Every other error path in this file passes `raw` through so downstream consumers still receive the data. This silently drops the hook input on any runtime error in the hook module.</violation>
</file>
<file name="scripts/lib/resolve-formatter.js">
<violation number="1" location="scripts/lib/resolve-formatter.js:143">
P2: The JSDoc `@returns` for `resolveFormatterBin` still promises `{{ bin: string, prefix: string[] }}` but this code path now returns `null`. Update the return annotation to `{{ bin: string, prefix: string[] } | null}` so future callers know to handle the null case.</violation>
</file>
<file name="scripts/hooks/quality-gate.js">
<violation number="1" location="scripts/hooks/quality-gate.js:113">
P2: The `gofmt -l` strict-check path doesn't detect command-level failures. If `gofmt` is not installed or otherwise errors out, `r.status` will be non-zero but `r.stdout` will be empty, so this branch silently passes. Add an `r.status` check for parity with the `fix` path above.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/hooks/post-edit-format.js`:
- Around line 36-55: Normalize the input path before invoking the formatter:
compute a resolvedFilePath = path.resolve(filePath) (use the same resolved path
that projectRoot was derived from) and use resolvedFilePath instead of filePath
when building args for Biome/Prettier and when calling execFileSync; update the
args construction (the array built in the formatter === 'biome' ? ... : ...) and
any other references to filePath (e.g., execFileSync invocation) to use
resolvedFilePath so the formatter always runs against the correct absolute path
while still running with cwd = projectRoot.
In `@scripts/hooks/quality-gate.js`:
- Around line 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).
In `@scripts/hooks/run-with-flags.js`:
- Around line 71-91: The current attempt to optimize by calling
require(scriptPath) is unsafe because legacy hooks run module-scope side effects
on require(), causing double execution; stop requiring arbitrary hook files and
always invoke hooks via the child-process fallback (the spawnSync path) instead
of using require(scriptPath) and hookModule.run, or alternatively only use
require() for hooks that explicitly export a safe run() and are marked safe—but
since existing hooks are unsafe, remove or bypass the require(scriptPath) block
and always call the spawnSync execution path for scriptPath/hookId to avoid
module-scope side effects and double-execution races.
In `@scripts/lib/resolve-formatter.js`:
- Around line 28-42: findProjectRoot currently only treats a directory with
package.json as the anchor which causes detectFormatter to miss repo-level
config files (e.g. biome.json, .prettierrc) and fall back to the leaf directory;
update findProjectRoot (and its projectRootCache usage) to walk parent
directories looking for common formatter/config files (at least biome.json and
.prettierrc variants) in addition to package.json, cache the discovered root for
startDir, and return that directory so detectFormatter can find top-level
configs even in repos without package.json.
- Around line 114-137: The code currently hardcodes npxBin (via isWin/npxBin) as
the fallback for both formatter === 'biome' and formatter === 'prettier', which
ignores CLAUDE_PACKAGE_MANAGER and other managers; update resolve-formatter.js
to determine the package-manager runner instead of using npxBin: replace uses of
npxBin when creating the fallback result for both the 'biome' and 'prettier'
branches with a call to the repo package-manager resolution logic (e.g., read
CLAUDE_PACKAGE_MANAGER or call the existing project/package-manager helper) so
the returned result.bin reflects the chosen runner (npm/npx, pnpm, yarn, bun)
while preserving binCache.set(cacheKey, result) and the prefix arrays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e6ff8ca-eea3-405f-8f79-383c8a93bc53
📒 Files selected for processing (6)
scripts/hooks/post-edit-format.jsscripts/hooks/quality-gate.jsscripts/hooks/run-with-flags.jsscripts/lib/resolve-formatter.jstests/lib/resolve-formatter.test.jstests/run-all.js
Comment on lines
+112
to
+116
| } else if (strict) { | ||
| const r = exec('gofmt', ['-l', filePath]); | ||
| if (r.stdout && r.stdout.trim()) { | ||
| log(`[QualityGate] gofmt check failed for ${filePath}`); | ||
| } |
Contributor
There was a problem hiding this comment.
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).
… edge cases - Only require() hooks that export run() to prevent module-scope side effects (stdin listeners, process.exit) from legacy hooks causing double execution or parent process termination - Normalize filePath via path.resolve() before passing to formatter args so relative paths work correctly with cwd: projectRoot - Add process.stdout.write(raw) in run() error catch for pass-through consistency with all other error paths - Add r.status check for gofmt -l strict path to detect command failures - Update JSDoc @returns to include null for resolveFormatterBin
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/hooks/run-with-flags.js (1)
101-114:⚠️ Potential issue | 🟠 MajorUse
process.execPathand explicitly handle spawn failures instead of silently defaulting to exit code 0.Line 102 uses the string
'node'instead ofprocess.execPath, which fails on systems wherenodeis not onPATH. WhenspawnSyncfails to spawn (e.g., command not found) or times out,result.erroris set orresult.signalis populated whileresult.statusisnull. Line 113 then defaults thisnullto0, silently treating spawn failures as success.Use
process.execPathto invoke the current Node runtime directly, and checkresult.errorto catch and report spawn failures explicitly rather than silently exiting with code 0.Suggested fix
- const result = spawnSync('node', [scriptPath], { + const result = spawnSync(process.execPath, [scriptPath], { input: raw, encoding: 'utf8', env: process.env, cwd: process.cwd(), timeout: 30000 }); + + if (result.error) { + process.stderr.write(`[Hook] ${hookId} failed for ${scriptPath}: ${result.error.message}\n`); + process.stdout.write(raw); + process.exit(0); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/run-with-flags.js` around lines 101 - 114, The hook child process invocation uses the literal 'node' and silently treats spawn failures as success; update the spawnSync call in this block to use process.execPath instead of 'node', then after spawnSync inspect result.error and result.signal (in addition to result.status): if result.error exists, write the error message to process.stderr and exit with a non-zero code (e.g., 1); if result.signal is set (timeout or killed) write a descriptive message to stderr and exit with a non-zero code; otherwise use result.status as the exit code. Ensure you still forward result.stdout/result.stderr to the parent before exiting.
♻️ Duplicate comments (3)
scripts/hooks/run-with-flags.js (1)
71-88:⚠️ Potential issue | 🟠 MajorThe source-text probe is not a safe boundary for
require().Lines 78-79 only prove the file mentions
module.exportsandrun; they do not prove that Line 83 can load it without module-scope side effects or thatrunis actually the exported entrypoint. I’d restrict directrequire()to a small allowlist of known-safe hooks (or another explicit safe-to-require contract) instead of gating it with regexes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/run-with-flags.js` around lines 71 - 88, The current regex probe (hasRunExport) is unsafe to decide require()-ing arbitrary hook files; change the logic in run-with-flags.js to only require(scriptPath) for hooks on an explicit allowlist (e.g., a Set or array of known-safe hook IDs) and otherwise fall back to the legacy spawnSync path; update the condition that currently uses hasRunExport to also check membership in that allowlist before attempting require(), and keep the existing try/catch that writes to process.stderr on require failure (using hookId, scriptPath, hookModule as needed).scripts/lib/resolve-formatter.js (2)
114-137:⚠️ Potential issue | 🟠 MajorThe fallback runner still hardcodes npm via
npx.When no local binary exists, both branches return
npx/npx.cmd, which bypasses the repo's package-manager selection and can break formatter execution in pnpm, yarn, or bun setups without npm shims. This should resolve the runner fromCLAUDE_PACKAGE_MANAGER/project config before building the fallback command.As per coding guidelines "
scripts/**/*.js: Support multiple package managers: npm, pnpm, yarn, bun, with configurable selection viaCLAUDE_PACKAGE_MANAGERenv var or project config".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/resolve-formatter.js` around lines 114 - 137, The code currently hardcodes npx/npx.cmd (npxBin) for fallback runners in resolve-formatter.js (branches handling formatter === 'biome' and formatter === 'prettier'); change this to resolve the package-manager runner from CLAUDE_PACKAGE_MANAGER (or project config) instead of always using npx: implement or call a helper like getPackageManagerRunner(projectRoot, process.env.CLAUDE_PACKAGE_MANAGER) that returns the correct runner binary and invocation prefix for npm/pnpm/yarn/bun (and handles isWin), then use that runner instead of npxBin when creating the fallback result objects (the ones assigned to result and cached in binCache with cacheKey), preserving existing prefix arrays (e.g., ['@biomejs/biome'] or ['prettier']) but applying the resolved runner invocation.
28-42:⚠️ Potential issue | 🟠 Major
findProjectRoot()still misses config-only repos.The upward walk stops at
package.json, so a repo that only hasbiome.jsonor.prettierrc*at the top level falls back to the leaf directory and both hooks quietly skip formatter detection. The root search needs to treat formatter config files as anchors too, or move the parent-directory walk intodetectFormatter().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/resolve-formatter.js` around lines 28 - 42, findProjectRoot() only treats package.json as a project anchor so repos that only have formatter configs (e.g., biome.json, .prettierrc, .prettierrc.json, .prettierrc.js, .prettierrc.yaml, .prettierrc.yml, .prettierrc.toml, etc.) are misdetected; update findProjectRoot (and use projectRootCache) to also consider those formatter config filenames during the upward directory walk and return the first directory containing any of them, or alternatively move the parent-directory walk into detectFormatter() and let detectFormatter() identify the nearest formatter-config-containing ancestor before caching the result in projectRootCache; ensure you reference/modify findProjectRoot, projectRootCache, and detectFormatter accordingly.
🧹 Nitpick comments (1)
scripts/hooks/quality-gate.js (1)
57-132: SplitmaybeRunQualityGate()into per-tool helpers.This function now owns formatter resolution plus Biome, Prettier, Go, and Ruff execution in one 70+ line block. Extracting small handlers here would make future tool-specific fixes safer and keeps the function within the repo's size/focus guideline.
As per coding guidelines "
**/*.{js,ts,tsx,jsx,go,py,java,cs,rb}: Keep functions small (under 50 lines) and focused. Keep files focused (under 800 lines maximum, typical 200-400 lines)".🤖 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 57 - 132, maybeRunQualityGate is too large and mixes formatter resolution with per-tool logic; extract per-tool helpers (e.g., runBiomeCheck(projectRoot, filePath, ext, fix, strict), runPrettierCheck(projectRoot, filePath, fix, strict), runGofmtCheck(filePath, fix, strict), runRuffCheck(filePath, fix, strict)) and move the specific exec/args/log handling into those functions while keeping maybeRunQualityGate limited to resolving filePath, ext, projectRoot, formatter (via findProjectRoot and detectFormatter) and then dispatching to the appropriate helper; reuse resolveFormatterBin, exec and log inside the helpers and return early from maybeRunQualityGate after dispatch to keep it under 50 lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/hooks/run-with-flags.js`:
- Around line 101-114: The hook child process invocation uses the literal 'node'
and silently treats spawn failures as success; update the spawnSync call in this
block to use process.execPath instead of 'node', then after spawnSync inspect
result.error and result.signal (in addition to result.status): if result.error
exists, write the error message to process.stderr and exit with a non-zero code
(e.g., 1); if result.signal is set (timeout or killed) write a descriptive
message to stderr and exit with a non-zero code; otherwise use result.status as
the exit code. Ensure you still forward result.stdout/result.stderr to the
parent before exiting.
---
Duplicate comments:
In `@scripts/hooks/run-with-flags.js`:
- Around line 71-88: The current regex probe (hasRunExport) is unsafe to decide
require()-ing arbitrary hook files; change the logic in run-with-flags.js to
only require(scriptPath) for hooks on an explicit allowlist (e.g., a Set or
array of known-safe hook IDs) and otherwise fall back to the legacy spawnSync
path; update the condition that currently uses hasRunExport to also check
membership in that allowlist before attempting require(), and keep the existing
try/catch that writes to process.stderr on require failure (using hookId,
scriptPath, hookModule as needed).
In `@scripts/lib/resolve-formatter.js`:
- Around line 114-137: The code currently hardcodes npx/npx.cmd (npxBin) for
fallback runners in resolve-formatter.js (branches handling formatter ===
'biome' and formatter === 'prettier'); change this to resolve the
package-manager runner from CLAUDE_PACKAGE_MANAGER (or project config) instead
of always using npx: implement or call a helper like
getPackageManagerRunner(projectRoot, process.env.CLAUDE_PACKAGE_MANAGER) that
returns the correct runner binary and invocation prefix for npm/pnpm/yarn/bun
(and handles isWin), then use that runner instead of npxBin when creating the
fallback result objects (the ones assigned to result and cached in binCache with
cacheKey), preserving existing prefix arrays (e.g., ['@biomejs/biome'] or
['prettier']) but applying the resolved runner invocation.
- Around line 28-42: findProjectRoot() only treats package.json as a project
anchor so repos that only have formatter configs (e.g., biome.json, .prettierrc,
.prettierrc.json, .prettierrc.js, .prettierrc.yaml, .prettierrc.yml,
.prettierrc.toml, etc.) are misdetected; update findProjectRoot (and use
projectRootCache) to also consider those formatter config filenames during the
upward directory walk and return the first directory containing any of them, or
alternatively move the parent-directory walk into detectFormatter() and let
detectFormatter() identify the nearest formatter-config-containing ancestor
before caching the result in projectRootCache; ensure you reference/modify
findProjectRoot, projectRootCache, and detectFormatter accordingly.
---
Nitpick comments:
In `@scripts/hooks/quality-gate.js`:
- Around line 57-132: maybeRunQualityGate is too large and mixes formatter
resolution with per-tool logic; extract per-tool helpers (e.g.,
runBiomeCheck(projectRoot, filePath, ext, fix, strict),
runPrettierCheck(projectRoot, filePath, fix, strict), runGofmtCheck(filePath,
fix, strict), runRuffCheck(filePath, fix, strict)) and move the specific
exec/args/log handling into those functions while keeping maybeRunQualityGate
limited to resolving filePath, ext, projectRoot, formatter (via findProjectRoot
and detectFormatter) and then dispatching to the appropriate helper; reuse
resolveFormatterBin, exec and log inside the helpers and return early from
maybeRunQualityGate after dispatch to keep it under 50 lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93dc38ca-addc-4ca5-a341-985efdb00a92
📒 Files selected for processing (4)
scripts/hooks/post-edit-format.jsscripts/hooks/quality-gate.jsscripts/hooks/run-with-flags.jsscripts/lib/resolve-formatter.js
pythonstrup
changed the title
pythonstrup
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Optimize the post-edit-format and quality-gate hooks for significantly faster execution by eliminating
npxoverhead, merging redundant Biome invocations, and reducing the process spawn chain.Benchmark Results
Changes
Shared
resolve-formatter.jsutility (scripts/lib/resolve-formatter.js— new)findProjectRoot(),detectFormatter(),resolveFormatterBin()into a shared modulenode_modules/.bin/biomeornode_modules/.bin/prettierbinary first, falls back tonpxEliminate npx overhead (
scripts/hooks/post-edit-format.js)npx(~200-500ms savings per invocation)biome format --write→biome check --write(format + lint in one pass)run()function for direct invocationSkip redundant quality-gate checks (
scripts/hooks/quality-gate.js).json/.mdfilesfindProjectRoot()instead ofprocess.cwd()for config detectiontimeout: 15000to prevent hangingDirect
require()invocation (scripts/hooks/run-with-flags.js)run(), calls it directly viarequire()instead ofspawnSync('node', ...)run()exportrequire()timeout: 30000to legacy spawnSync pathrequire()errors to stderr instead of silently swallowingType of Change
feat:New featurefix:Bug fixTesting
Unit tests:
resolve-formatter.js— all passLive E2E tests with real projects:
"✓'✓26 comprehensive test cases covering:
Checklist
node tests/run-all.js)Summary by cubic
Speed up formatter hooks by using local binaries, merging
biomeruns, and calling hooks via directrequire(). Format+lint after edits now runs ~52x faster (≈3300ms → ~63ms), and the duplicatebiomecheck in the quality gate is removed.Refactors
scripts/lib/resolve-formatter.jswith caching to find project root, detectbiome/prettier(incl.package.json“prettier” key), and resolve localnode_modules/.binbeforenpx.post-edit-formatandquality-gate: use local bins, mergebiomeinto a singlecheck --writepost-edit; skip JS/TS in quality gate; still check.json/.md; normalize file paths to absolute; both exportrun()for direct invocation.run-with-flags: directly invokes hooks that exportrun(); legacy hooks fall back to spawn.Bug Fixes
quality-gate: correct root detection withfindProjectRoot(), resolve file paths to absolute, add a 15s timeout, and improve strict-modegofmtlogging with failure detection.run-with-flags: add path traversal guard; onlyrequire()hooks that exportrun(); separaterequire()andrun()error handling, logrequire()errors to stderr, write input back onrun()errors, and add a 30s timeout for the legacy spawn path.Written for commit 3abd3bf. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor
Tests