fix(hooks): respect package manager formatter fallback#365
fix(hooks): respect package manager formatter fallback#365tsubasakong wants to merge 2 commits intoaffaan-m:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR implements Windows-aware and project-aware command resolution for formatters. It replaces hard-coded npx usage with package-manager-specific runners (npm, pnpm, yarn, bun), respecting the CLAUDE_PACKAGE_MANAGER environment variable and project configuration. Changes
Sequence DiagramsequenceDiagram
participant Hook as post-edit-format Hook
participant Resolver as getFormatterCommand()
participant RunnerFactory as getFormatterRunner()
participant PkgMgr as getPackageManager()
participant Formatter as biome/prettier
Hook->>Resolver: Get command for file
Resolver->>RunnerFactory: Build runner for project
RunnerFactory->>PkgMgr: Detect package manager<br/>(CLAUDE_PACKAGE_MANAGER or project config)
PkgMgr-->>RunnerFactory: npm | pnpm | yarn | bun
RunnerFactory->>RunnerFactory: Map to runner:<br/>npm→npx, pnpm→pnpm dlx,<br/>yarn→yarn dlx, bun→bunx
RunnerFactory->>RunnerFactory: Handle Windows .cmd suffix
RunnerFactory-->>Resolver: {bin, prefix}
Resolver->>Resolver: Construct full command<br/>with runner bin + prefix
Resolver-->>Hook: [bin, prefix, formatter, args...]
Hook->>Formatter: Execute formatter command
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
tsubasakong
force-pushed
the
lucas/opener-a-affaan-m-everything-claude-code-363
branch
from
e1e84f5 to
6f06ffd
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hooks/hooks.test.js`:
- Around line 78-90: The current createCommandShim produces a POSIX shell script
which breaks Windows CI because getRunnerBin on Windows expects .cmd
executables; update createCommandShim to detect process.platform === 'win32' and
when true create a .cmd shim (append .cmd to shimPath if needed) whose content
uses Windows batch syntax to echo the command name and each %* arg into the
JSON-stringified logFile, otherwise keep the existing POSIX script for
non-Windows; ensure the created shim has the correct filename (with .cmd on
Windows) and is written with appropriate permissions (fs.writeFileSync) so tests
invoking getRunnerBin find and execute the shim on all platforms.
- Around line 746-774: The test currently asserts the shim invocation is exactly
'bunx' but on Windows the shim will be 'bunx.cmd'; update the assertion in the
asyncTest 'uses project package manager config for formatter fallback' to accept
the Windows variant by checking process.platform === 'win32' and expecting
'bunx.cmd' (or compute expectedBin = process.platform === 'win32' ? 'bunx.cmd' :
'bunx') then assert.deepStrictEqual(logLines, [expectedBin, '@biomejs/biome',
'format', '--write', testFile]) so the test passes cross-platform; modify the
assertion near the assert.deepStrictEqual call that references logLines
accordingly.
- Around line 718-744: The test fails on Windows because the pnpm shim is
created as 'pnpm' but the process looks for 'pnpm.cmd'; update the test to use
the platform-aware runner name instead of a hardcoded 'pnpm' by creating the
shim with getRunnerBin('pnpm') (i.e., call createCommandShim(binDir,
getRunnerBin('pnpm'), logFile)) and change the assertion that checks the logged
command to compare against getRunnerBin('pnpm') (or accept both 'pnpm' and
'pnpm.cmd') so post-edit-format.js and the test both use the same platform-aware
executable name; references: createCommandShim, getRunnerBin,
post-edit-format.js, logFile, and the assertion that currently expects ['pnpm',
'dlx', 'prettier', '--write', testFile].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e668100-7879-4500-a854-ffe22cc1abdc
📒 Files selected for processing (2)
scripts/hooks/post-edit-format.jstests/hooks/hooks.test.js
| function createCommandShim(binDir, name, logFile) { | ||
| const shimPath = path.join(binDir, name); | ||
| const script = `#!/bin/sh | ||
| { | ||
| printf '%s\n' "$(basename "$0")" | ||
| for arg in "$@"; do | ||
| printf '%s\n' "$arg" | ||
| done | ||
| } > ${JSON.stringify(logFile)} | ||
| `; | ||
| fs.writeFileSync(shimPath, script, { mode: 0o755 }); | ||
| return shimPath; | ||
| } |
There was a problem hiding this comment.
Cross-platform issue: POSIX-only shims cause Windows CI failures.
This createCommandShim function creates POSIX shell scripts, but on Windows:
getRunnerBinreturns.cmdsuffixed binaries (e.g.,pnpm.cmd)- The shim is created without the
.cmdextension - The shim content uses POSIX shell syntax (
#!/bin/sh,printf,for arg in "$@")
This is the root cause of the pipeline failures:
ENOENT: no such file or directory, open '...\pnpm.log'
ENOENT: no such file or directory, open '...\bunx.log'
🔧 Proposed fix: Platform-aware shim creation
function createCommandShim(binDir, name, logFile) {
- const shimPath = path.join(binDir, name);
- const script = `#!/bin/sh
+ const isWindows = process.platform === 'win32';
+ const shimPath = path.join(binDir, isWindows ? `${name}.cmd` : name);
+
+ const script = isWindows
+ ? `@echo off
+echo %~n0> "${logFile}"
+:loop
+if "%~1"=="" goto done
+echo %~1>> "${logFile}"
+shift
+goto loop
+:done
+`
+ : `#!/bin/sh
{
printf '%s\\n' "$(basename "$0")"
for arg in "$@"; do
printf '%s\\n' "$arg"
done
} > ${JSON.stringify(logFile)}
`;
fs.writeFileSync(shimPath, script, { mode: 0o755 });
return shimPath;
}As per coding guidelines: "Ensure cross-platform compatibility for Windows, macOS, and Linux via Node.js scripts".
📝 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.
| function createCommandShim(binDir, name, logFile) { | |
| const shimPath = path.join(binDir, name); | |
| const script = `#!/bin/sh | |
| { | |
| printf '%s\n' "$(basename "$0")" | |
| for arg in "$@"; do | |
| printf '%s\n' "$arg" | |
| done | |
| } > ${JSON.stringify(logFile)} | |
| `; | |
| fs.writeFileSync(shimPath, script, { mode: 0o755 }); | |
| return shimPath; | |
| } | |
| function createCommandShim(binDir, name, logFile) { | |
| const isWindows = process.platform === 'win32'; | |
| const shimPath = path.join(binDir, isWindows ? `${name}.cmd` : name); | |
| const script = isWindows | |
| ? `@echo off | |
| echo %~n0> "${logFile}" | |
| :loop | |
| if "%~1"=="" goto done | |
| echo %~1>> "${logFile}" | |
| shift | |
| goto loop | |
| :done | |
| ` | |
| : `#!/bin/sh | |
| { | |
| printf '%s\n' "$(basename "$0")" | |
| for arg in "$@"; do | |
| printf '%s\n' "$arg" | |
| done | |
| } > ${JSON.stringify(logFile)} | |
| `; | |
| fs.writeFileSync(shimPath, script, { mode: 0o755 }); | |
| return shimPath; | |
| } |
🤖 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 78 - 90, The current
createCommandShim produces a POSIX shell script which breaks Windows CI because
getRunnerBin on Windows expects .cmd executables; update createCommandShim to
detect process.platform === 'win32' and when true create a .cmd shim (append
.cmd to shimPath if needed) whose content uses Windows batch syntax to echo the
command name and each %* arg into the JSON-stringified logFile, otherwise keep
the existing POSIX script for non-Windows; ensure the created shim has the
correct filename (with .cmd on Windows) and is written with appropriate
permissions (fs.writeFileSync) so tests invoking getRunnerBin find and execute
the shim on all platforms.
| if (await asyncTest('uses CLAUDE_PACKAGE_MANAGER runner for formatter fallback', async () => { | ||
| const testDir = createTestDir(); | ||
| const binDir = path.join(testDir, 'bin'); | ||
| const logFile = path.join(testDir, 'pnpm.log'); | ||
| fs.mkdirSync(binDir, { recursive: true }); | ||
| createCommandShim(binDir, 'pnpm', logFile); | ||
|
|
||
| const testFile = path.join(testDir, 'src', 'example.ts'); | ||
| fs.mkdirSync(path.dirname(testFile), { recursive: true }); | ||
| fs.writeFileSync(path.join(testDir, 'package.json'), JSON.stringify({ name: 'pm-env-test' })); | ||
| fs.writeFileSync(path.join(testDir, '.prettierrc'), '{}'); | ||
| fs.writeFileSync(testFile, 'const answer=42;\n'); | ||
|
|
||
| try { | ||
| const stdinJson = JSON.stringify({ tool_input: { file_path: testFile } }); | ||
| const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson, { | ||
| CLAUDE_PACKAGE_MANAGER: 'pnpm', | ||
| PATH: `${binDir}${path.delimiter}${process.env.PATH || ''}` | ||
| }); | ||
|
|
||
| assert.strictEqual(result.code, 0, 'Should exit 0 with pnpm fallback'); | ||
| const logLines = fs.readFileSync(logFile, 'utf8').trim().split('\n'); | ||
| assert.deepStrictEqual(logLines, ['pnpm', 'dlx', 'prettier', '--write', testFile]); | ||
| } finally { | ||
| cleanupTestDir(testDir); | ||
| } | ||
| })) passed++; else failed++; |
There was a problem hiding this comment.
Test will fail on Windows due to platform-specific shim issue.
The test creates a pnpm shim, but on Windows getRunnerBin('pnpm') returns pnpm.cmd. The execFileSync call in post-edit-format.js will search for pnpm.cmd but won't find it since the shim is created as pnpm (no .cmd extension).
Additionally, the assertion at Line 740 expects ['pnpm', 'dlx', 'prettier', '--write', testFile], but on Windows the first element would be pnpm.cmd if the shim were created correctly.
🔧 Proposed fix: Handle Windows in test assertion
assert.strictEqual(result.code, 0, 'Should exit 0 with pnpm fallback');
const logLines = fs.readFileSync(logFile, 'utf8').trim().split('\n');
- assert.deepStrictEqual(logLines, ['pnpm', 'dlx', 'prettier', '--write', testFile]);
+ const expectedBin = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm';
+ assert.deepStrictEqual(logLines, [expectedBin, 'dlx', 'prettier', '--write', testFile]);🤖 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 718 - 744, The test fails on Windows
because the pnpm shim is created as 'pnpm' but the process looks for 'pnpm.cmd';
update the test to use the platform-aware runner name instead of a hardcoded
'pnpm' by creating the shim with getRunnerBin('pnpm') (i.e., call
createCommandShim(binDir, getRunnerBin('pnpm'), logFile)) and change the
assertion that checks the logged command to compare against getRunnerBin('pnpm')
(or accept both 'pnpm' and 'pnpm.cmd') so post-edit-format.js and the test both
use the same platform-aware executable name; references: createCommandShim,
getRunnerBin, post-edit-format.js, logFile, and the assertion that currently
expects ['pnpm', 'dlx', 'prettier', '--write', testFile].
| if (await asyncTest('uses project package manager config for formatter fallback', async () => { | ||
| const testDir = createTestDir(); | ||
| const binDir = path.join(testDir, 'bin'); | ||
| const logFile = path.join(testDir, 'bunx.log'); | ||
| fs.mkdirSync(binDir, { recursive: true }); | ||
| fs.mkdirSync(path.join(testDir, '.claude'), { recursive: true }); | ||
| createCommandShim(binDir, 'bunx', logFile); | ||
|
|
||
| const testFile = path.join(testDir, 'src', 'example.ts'); | ||
| fs.mkdirSync(path.dirname(testFile), { recursive: true }); | ||
| fs.writeFileSync(path.join(testDir, 'package.json'), JSON.stringify({ name: 'pm-project-test' })); | ||
| fs.writeFileSync(path.join(testDir, 'biome.json'), '{}'); | ||
| fs.writeFileSync(path.join(testDir, '.claude', 'package-manager.json'), JSON.stringify({ packageManager: 'bun' })); | ||
| fs.writeFileSync(testFile, 'const answer=42;\n'); | ||
|
|
||
| try { | ||
| const stdinJson = JSON.stringify({ tool_input: { file_path: testFile } }); | ||
| const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson, { | ||
| CLAUDE_PACKAGE_MANAGER: '', | ||
| PATH: `${binDir}${path.delimiter}${process.env.PATH || ''}` | ||
| }); | ||
|
|
||
| assert.strictEqual(result.code, 0, 'Should exit 0 with project-config fallback'); | ||
| const logLines = fs.readFileSync(logFile, 'utf8').trim().split('\n'); | ||
| assert.deepStrictEqual(logLines, ['bunx', '@biomejs/biome', 'format', '--write', testFile]); | ||
| } finally { | ||
| cleanupTestDir(testDir); | ||
| } | ||
| })) passed++; else failed++; |
There was a problem hiding this comment.
Same cross-platform issue affects the bunx test.
The bunx shim has the same problem—created without .cmd extension and with POSIX shell syntax. The assertion at Line 770 also needs to account for bunx.cmd on Windows.
🔧 Proposed fix: Handle Windows in test assertion
assert.strictEqual(result.code, 0, 'Should exit 0 with project-config fallback');
const logLines = fs.readFileSync(logFile, 'utf8').trim().split('\n');
- assert.deepStrictEqual(logLines, ['bunx', '@biomejs/biome', 'format', '--write', testFile]);
+ const expectedBin = process.platform === 'win32' ? 'bunx.cmd' : 'bunx';
+ assert.deepStrictEqual(logLines, [expectedBin, '@biomejs/biome', 'format', '--write', testFile]);🤖 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 746 - 774, The test currently asserts
the shim invocation is exactly 'bunx' but on Windows the shim will be
'bunx.cmd'; update the assertion in the asyncTest 'uses project package manager
config for formatter fallback' to accept the Windows variant by checking
process.platform === 'win32' and expecting 'bunx.cmd' (or compute expectedBin =
process.platform === 'win32' ? 'bunx.cmd' : 'bunx') then
assert.deepStrictEqual(logLines, [expectedBin, '@biomejs/biome', 'format',
'--write', testFile]) so the test passes cross-platform; modify the assertion
near the assert.deepStrictEqual call that references logLines accordingly.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
2 participants
Description
pnpm dlxFixes #363
Validation:
Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesChecklist
node tests/run-all.js)