refactor(hooks): simplify Biome detection with process.cwd()#281
Closed
pythonstrup wants to merge 2 commits intoaffaan-m:mainfrom
Closed
refactor(hooks): simplify Biome detection with process.cwd()#281pythonstrup wants to merge 2 commits intoaffaan-m:mainfrom
pythonstrup wants to merge 2 commits intoaffaan-m:mainfrom
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces heuristic formatter detection with a runner-based execution model that selects an executable via CLAUDE_PACKAGE_MANAGER (or npm), uses process.cwd() for Biome vs Prettier detection, limits processing to JS/TS files, and invokes the chosen formatter via execFileSync with constructed bin+args. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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
🤖 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 40-43: The hook currently hardcodes npx via npxBin; change it to
honor the CLAUDE_PACKAGE_MANAGER env var (or project config) by mapping package
manager names to the correct runner and runner-args (e.g., npm -> ['npx'] or
['npm','exec','--'], pnpm -> ['pnpm','dlx'], yarn -> ['yarn','dlx'], bun ->
['bun','x']) and use that runner instead of npxBin; update the code that builds
args (the hasBiome branch and the prettier branch that use filePath) to prepend
the selected runner prefix (and handle Windows filename variants when needed) so
that commands like running `@biomejs/biome` or prettier are executed via the
configured package manager rather than always via npx.
In `@tests/hooks/hooks.test.js`:
- Around line 3669-3709: The tests create biome.json/biome.jsonc in testDir but
call runScript without setting its working directory, so process.cwd() inside
post-edit-format.js remains the test runner's cwd; update the three affected
tests to pass a cwd option to runScript (e.g., runScript(path.join(scriptsDir,
'post-edit-format.js'), stdinJson, { cwd: testDir })) so the child process runs
with testDir as its cwd (references: runScript, createTestDir, cleanupTestDir,
post-edit-format.js); keep cleanupTestDir(testDir) after the run.
Comment on lines
+3669
to
+3709
| if (await asyncTest('uses biome when biome.json exists in cwd', async () => { | ||
| const testDir = createTestDir(); | ||
| fs.writeFileSync(path.join(testDir, 'biome.json'), '{}'); | ||
| const testFile = path.join(testDir, 'app.ts'); | ||
| fs.writeFileSync(testFile, 'const x = 1;'); | ||
|
|
||
| const stdinJson = JSON.stringify({ tool_input: { file_path: testFile } }); | ||
| const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson); | ||
| assert.strictEqual(result.code, 0, 'Should exit 0 with biome.json in cwd'); | ||
| assert.ok(result.stdout.includes('tool_input'), 'Should pass through stdin data'); | ||
| cleanupTestDir(testDir); | ||
| })) passed++; else failed++; | ||
|
|
||
| if (await asyncTest('uses biome when biome.jsonc exists in cwd', async () => { | ||
| const testDir = createTestDir(); | ||
| fs.writeFileSync(path.join(testDir, 'biome.jsonc'), '{}'); | ||
| const testFile = path.join(testDir, 'app.tsx'); | ||
| fs.writeFileSync(testFile, 'const x = 1;'); | ||
|
|
||
| const stdinJson = JSON.stringify({ tool_input: { file_path: testFile } }); | ||
| const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson); | ||
| assert.strictEqual(result.code, 0, 'Should exit 0 with biome.jsonc in cwd'); | ||
| assert.ok(result.stdout.includes('tool_input'), 'Should pass through stdin data'); | ||
| cleanupTestDir(testDir); | ||
| })) passed++; else failed++; | ||
|
|
||
| if (await asyncTest('source uses process.cwd() for biome detection, not walk-up', async () => { | ||
| const formatSource = fs.readFileSync(path.join(scriptsDir, 'post-edit-format.js'), 'utf8'); | ||
| assert.ok(formatSource.includes('process.cwd()'), 'Should use process.cwd() for biome detection'); | ||
| assert.ok(!formatSource.includes('findBiomeRoot'), 'Should not have findBiomeRoot walk-up function'); | ||
| })) passed++; else failed++; | ||
|
|
||
| if (await asyncTest('source contains biome check --write and BIOME_CONFIGS covers both extensions', async () => { | ||
| const formatSource = fs.readFileSync(path.join(scriptsDir, 'post-edit-format.js'), 'utf8'); | ||
| assert.ok(formatSource.includes('@biomejs/biome'), 'Should reference @biomejs/biome package'); | ||
| assert.ok(formatSource.includes('check'), 'Should include biome check subcommand'); | ||
| assert.ok(formatSource.includes('--write'), 'Should include --write flag'); | ||
| assert.ok(formatSource.includes('biome.json'), 'BIOME_CONFIGS should include biome.json'); | ||
| assert.ok(formatSource.includes('biome.jsonc'), 'BIOME_CONFIGS should include biome.jsonc'); | ||
| })) passed++; else failed++; | ||
|
|
Contributor
There was a problem hiding this comment.
Biome behavior tests don’t execute from the Biome-configured cwd.
These tests create biome.json/biome.jsonc in testDir, but runScript(...) is called without a cwd override, so the hook still evaluates process.cwd() from the test runner’s cwd. That makes these checks pass even if cwd-based Biome detection is broken.
🧪 Proposed fix
-function runScript(scriptPath, input = '', env = {}) {
+function runScript(scriptPath, input = '', env = {}, options = {}) {
return new Promise((resolve, reject) => {
const proc = spawn('node', [scriptPath], {
env: { ...process.env, ...env },
- stdio: ['pipe', 'pipe', 'pipe']
+ stdio: ['pipe', 'pipe', 'pipe'],
+ cwd: options.cwd || process.cwd(),
});-const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson);
+const result = await runScript(
+ path.join(scriptsDir, 'post-edit-format.js'),
+ stdinJson,
+ {},
+ { cwd: testDir }
+);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hooks/hooks.test.js` around lines 3669 - 3709, The tests create
biome.json/biome.jsonc in testDir but call runScript without setting its working
directory, so process.cwd() inside post-edit-format.js remains the test runner's
cwd; update the three affected tests to pass a cwd option to runScript (e.g.,
runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson, { cwd:
testDir })) so the child process runs with testDir as its cwd (references:
runScript, createTestDir, cleanupTestDir, post-edit-format.js); keep
cleanupTestDir(testDir) after the run.
affaan-m
requested changes
Feb 25, 2026
Owner
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
Replace walk-up directory traversal (findBiomeRoot) with simple process.cwd() check for biome.json/biome.jsonc. Claude Code already runs from the project root, so walking up from the edited file is unnecessary complexity.
pythonstrup
force-pushed
the
feat/refactor-post-edit-hooks
branch
from
852d64a to
acaeea4
Compare
dlx (pnpm dlx, yarn dlx) downloads packages from registry on every invocation, ignoring locally installed versions. Since this hook runs on every file edit, dlx causes unnecessary network requests, slow formatting, offline failures, and potential version mismatches. Switch to exec (pnpm exec, yarn exec) which resolves from local node_modules first, matching npx/bunx behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/hooks/post-edit-format.js (1)
29-30: NormalizeCLAUDE_PACKAGE_MANAGERbefore lookup.Case or whitespace in env values (e.g.,
PNPM,yarn) currently falls back to npm. Normalizing avoids surprising runner selection.Suggested fix
function getRunner() { - const pm = process.env.CLAUDE_PACKAGE_MANAGER; + const pm = process.env.CLAUDE_PACKAGE_MANAGER?.trim().toLowerCase(); const runner = pm && RUNNERS[pm] ? RUNNERS[pm] : RUNNERS.npm;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/hooks/post-edit-format.js` around lines 29 - 30, The env lookup for CLAUDE_PACKAGE_MANAGER currently uses the raw value and can miss matches due to case or surrounding whitespace; normalize the variable by reading process.env.CLAUDE_PACKAGE_MANAGER into pm, then trim whitespace and convert to lower-case before using it to index RUNNERS (so the pm lookup in the runner assignment uses the normalized value); update the variable referenced in the runner assignment (pm, RUNNERS, runner) to use this normalized string to ensure correct selection of npm, pnpm, yarn, or bun.
🤖 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 25-33: The RUNNERS entry for Bun currently uses bin: 'bunx' which
causes getRunner's Windows codepath to append '.cmd' and rely on a
non-guaranteed bunx shim; update the RUNNERS.bun entry to use bin: 'bun' and set
args: ['x'] instead (so getRunner returns runner.bin === 'bun' and runner.args
=== ['x']), ensuring any existing logic in getRunner that appends '.cmd' still
works but now invokes Bun via "bun x" cross-platform; change only the RUNNERS
object (and any tests/config that reference RUNNERS.bun) to this pair of
properties.
---
Nitpick comments:
In `@scripts/hooks/post-edit-format.js`:
- Around line 29-30: The env lookup for CLAUDE_PACKAGE_MANAGER currently uses
the raw value and can miss matches due to case or surrounding whitespace;
normalize the variable by reading process.env.CLAUDE_PACKAGE_MANAGER into pm,
then trim whitespace and convert to lower-case before using it to index RUNNERS
(so the pm lookup in the runner assignment uses the normalized value); update
the variable referenced in the runner assignment (pm, RUNNERS, runner) to use
this normalized string to ensure correct selection of npm, pnpm, yarn, or bun.
Comment on lines
+25
to
+33
| bun: { bin: 'bunx', args: [] }, | ||
| }; | ||
|
|
||
| function getRunner() { | ||
| const pm = process.env.CLAUDE_PACKAGE_MANAGER; | ||
| const runner = pm && RUNNERS[pm] ? RUNNERS[pm] : RUNNERS.npm; | ||
| const bin = process.platform === 'win32' && !runner.bin.endsWith('.cmd') | ||
| ? `${runner.bin}.cmd` | ||
| : runner.bin; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Bun bunx command Windows support bunx.cmd shim official documentation
💡 Result:
- Windows support (official): Bun supports Windows (x64); the installer docs state Windows 10 version 1809 or later is required. [1]
bunx(official):bunxis an alias forbun xand is auto-installed when you installbun(Bun’s equivalent ofnpx/yarn dlx). [2]bunx.cmdshim: Bun’s official docs do not document abunx.cmdshim. Thebunx.cmd“shim” behavior is discussed as a Windows packaging/installation quirk (e.g., Scoop creatingbunx.cmdeven thoughbunxis an.exe) in Bun’s issue tracker. [3]
Sources: [1] Bun Installation docs (Windows requirements) • [2] Bun bunx CLI docs • [3] oven-sh/bun issue #10599 (bunx.cmd shim discussion)
🏁 Script executed:
# Check the actual structure of the file to confirm the code context
cat -n scripts/hooks/post-edit-format.js | head -40Repository: affaan-m/everything-claude-code
Length of output: 1523
Use bun x form to avoid Windows compatibility risk.
RUNNERS.bun uses bunx, and the Windows code path (line 31–33) appends .cmd to create bunx.cmd. However, Bun's official documentation does not guarantee that a bunx.cmd shim exists; this is a packaging quirk handled by some installers (e.g., Scoop) but not guaranteed by Bun itself. If the shim is missing, Bun formatting silently fails on Windows, violating the cross-platform compatibility requirement.
Use bin: 'bun' with args: ['x'] instead, which aligns with Bun's official behavior and avoids the undocumented shim dependency.
Suggested fix
const RUNNERS = {
npm: { bin: 'npx', args: [] },
pnpm: { bin: 'pnpm', args: ['dlx'] },
yarn: { bin: 'yarn', args: ['dlx'] },
- bun: { bin: 'bunx', args: [] },
+ bun: { bin: 'bun', args: ['x'] },
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bun: { bin: 'bunx', args: [] }, | |
| }; | |
| function getRunner() { | |
| const pm = process.env.CLAUDE_PACKAGE_MANAGER; | |
| const runner = pm && RUNNERS[pm] ? RUNNERS[pm] : RUNNERS.npm; | |
| const bin = process.platform === 'win32' && !runner.bin.endsWith('.cmd') | |
| ? `${runner.bin}.cmd` | |
| : runner.bin; | |
| bun: { bin: 'bun', args: ['x'] }, | |
| }; | |
| function getRunner() { | |
| const pm = process.env.CLAUDE_PACKAGE_MANAGER; | |
| const runner = pm && RUNNERS[pm] ? RUNNERS[pm] : RUNNERS.npm; | |
| const bin = process.platform === 'win32' && !runner.bin.endsWith('.cmd') | |
| ? `${runner.bin}.cmd` | |
| : runner.bin; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/post-edit-format.js` around lines 25 - 33, The RUNNERS entry
for Bun currently uses bin: 'bunx' which causes getRunner's Windows codepath to
append '.cmd' and rely on a non-guaranteed bunx shim; update the RUNNERS.bun
entry to use bin: 'bun' and set args: ['x'] instead (so getRunner returns
runner.bin === 'bun' and runner.args === ['x']), ensuring any existing logic in
getRunner that appends '.cmd' still works but now invokes Bun via "bun x"
cross-platform; change only the RUNNERS object (and any tests/config that
reference RUNNERS.bun) to this pair of properties.
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
2 participants
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.
Summary
Simplifies the Biome detection logic in
post-edit-format.jsintroduced by #252.process.cwd()— reduces 3 functions (46 lines) to 3 inline linesexistsSynccalls (walking up to/) to just 1-2 checkspackage.jsonas project root;process.cwd()matches the actual working directory Claude Code runs inbiome check --writeinstead ofbiome format --writefor formatting + linting in one passChanged Files
scripts/hooks/post-edit-format.jsfindProjectRoot+detectFormatter+getFormatterCommandwith inlineprocess.cwd()detectiontests/hooks/hooks.test.jsTest plan
node tests/run-all.jspassesbiome.json🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests