Repository navigation
fix(e2e): prevent fixture hook mutations - #2820
Merged
Merged
Conversation
📝 WalkthroughWalkthroughFixture dependency installation is centralized through a configurable pnpm helper that adds guards against parent Git-hook changes. Five app setup routines now use the helper, and regression tests verify invocation, environment propagation, and unchanged parent Git state. ChangesFixture install safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AppSetup
participant installPnpmDependencies
participant npx
participant FixtureGitScripts
AppSetup->>installPnpmDependencies: provide fixture directory and options
installPnpmDependencies->>npx: run pnpm@10 install with guarded environment
npx->>FixtureGitScripts: execute install lifecycle scripts
FixtureGitScripts-->>npx: skip parent hook mutation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
vize
@vizejs/fresco
@vizejs/musea-mcp-server
oxlint-plugin-vize
@vizejs/rspack-plugin
@vizejs/unplugin
@vizejs/vite-plugin
@vizejs/vite-plugin-musea
@vizejs/musea-nuxt
@vizejs/nuxt
commit: |
ubugeeei
force-pushed
the
agent/prevent-fixture-hook-mutations
branch
from
July 12, 2026 05:51
b4b1f75 to
7efdc44
Compare
Contributor
Detailed Test ReportCommit: Area Summary
Test InventoryTotal tracked cases: 8767 across 1265 files.
Files
Comment truncated at 64000 characters. Open the workflow run for the full job log. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/tooling/playwright-app-config.test.ts`:
- Around line 27-37: Replace the implementation-dependent assertions in the test
for external fixture installs with focused runtime coverage of the installer
behavior. Exercise the relevant helper, verify the child process receives the
hook-safety environment, and assert the parent .git/config and .git/hooks remain
unchanged; retain any source-policy matching only as supplemental coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a7e82e80-1530-4b15-9741-9d5f76e99c9e
📒 Files selected for processing (2)
tests/_helpers/apps.tstests/tooling/playwright-app-config.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (26)
- GitHub Check: Normalize titles and assignees
- GitHub Check: CodeRabbit
- GitHub Check: cargo-semver-checks (vize_carton)
- GitHub Check: cargo-semver-checks (vize_relief)
- GitHub Check: tool-benchmark
- GitHub Check: cargo-semver-checks (vize_fresco)
- GitHub Check: cargo-semver-checks (vize_atelier_dom)
- GitHub Check: cargo-semver-checks (vize_atelier_sfc)
- GitHub Check: cargo-semver-checks (vize_atelier_vapor)
- GitHub Check: cargo-semver-checks (vize_musea)
- GitHub Check: cargo-semver-checks (vize_atelier_core)
- GitHub Check: build-js-packages
- GitHub Check: cargo-semver-checks (vize_croquis)
- GitHub Check: clippy-and-test
- GitHub Check: cargo-semver-checks (vize_atelier_ssr)
- GitHub Check: miri
- GitHub Check: vue-parity
- GitHub Check: coverage
- GitHub Check: test-js-packages
- GitHub Check: security-audit
- GitHub Check: Publish preview packages
- GitHub Check: check-vize-apps
- GitHub Check: test-scripts
- GitHub Check: pr-benchmark
- GitHub Check: editor-extensions
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (1)
tests/tooling/**
⚙️ CodeRabbit configuration file
tests/tooling/**: Ensure workflow and release tests assert observable behavior rather than incidental formatting. Prefer focused assertions that keep PRs small and failures easy to diagnose.
Files:
tests/tooling/playwright-app-config.test.ts
🔇 Additional comments (2)
tests/_helpers/apps.ts (2)
160-160: 🩺 Stability & AvailabilityVerify the
execFileSync("npx", ...)change on Windows.If
npxresolves to a.cmdshim, this directexecFileSynccall can fail because Node does not launch Windows.cmdfiles throughexecFilewithout a shell. Verify the supported platforms and usenpx.cmd/cmd.exeon Windows if required. (nodejs.org)
148-159: LGTM!Also applies to: 161-169, 766-769, 896-904, 1255-1259, 1561-1564, 1729-1732
Contributor
PR BenchmarkBase:
Raw run timesCompile SFC
Lint
Format
Type check (1T)
Type check (max)
|
Contributor
Detailed Test ReportCommit: Area Summary
Test InventoryTotal tracked cases: 8767 across 1265 files.
Files
Comment truncated at 64000 characters. Open the workflow run for the full job log. |
96 tasks done
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/_helpers/apps.ts`:
- Around line 154-171: Update installPnpmDependencies to resolve the npx
executable with a Windows-specific fallback, such as selecting the .cmd shim
when running on Windows while preserving the existing command elsewhere. Keep
the current arguments, environment, timeout, and stdio behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6cf5f8c4-2b56-4a22-bc23-7956be559268
📒 Files selected for processing (2)
tests/_helpers/apps.tstests/tooling/playwright-app-config.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (23)
- GitHub Check: CodeRabbit
- GitHub Check: cargo-semver-checks (vize_croquis)
- GitHub Check: cargo-semver-checks (vize_fresco)
- GitHub Check: cargo-semver-checks (vize_atelier_core)
- GitHub Check: cargo-semver-checks (vize_relief)
- GitHub Check: cargo-semver-checks (vize_musea)
- GitHub Check: cargo-semver-checks (vize_atelier_vapor)
- GitHub Check: cargo-semver-checks (vize_carton)
- GitHub Check: cargo-semver-checks (vize_atelier_sfc)
- GitHub Check: cargo-semver-checks (vize_atelier_ssr)
- GitHub Check: cargo-semver-checks (vize_atelier_dom)
- GitHub Check: cargo-semver-checks (vize_armature)
- GitHub Check: test-js-packages
- GitHub Check: check-vize-apps
- GitHub Check: vue-parity
- GitHub Check: test-scripts
- GitHub Check: editor-extensions
- GitHub Check: clippy-and-test
- GitHub Check: coverage
- GitHub Check: security-audit
- GitHub Check: build-js-packages
- GitHub Check: tool-benchmark
- GitHub Check: pr-benchmark
🧰 Additional context used
📓 Path-based instructions (1)
tests/tooling/**
⚙️ CodeRabbit configuration file
tests/tooling/**: Ensure workflow and release tests assert observable behavior rather than incidental formatting. Prefer focused assertions that keep PRs small and failures easy to diagnose.
Files:
tests/tooling/playwright-app-config.test.ts
🔇 Additional comments (3)
tests/tooling/playwright-app-config.test.ts (1)
26-39: Still asserting source formatting, not runtime hook-safety behavior.This duplicates a previously raised concern: the test extracts
installPnpmDependencies's source text and regex-matches literal spellings (execFileSync("npx", args, key ordering, string keys) instead of invoking the helper and verifying the child process environment / that the parent.git/configand.git/hooksare untouched. It will pass under a behavior-preserving refactor that changes formatting, and won't catch a new raw install call that bypasses this exact regex.As per path instructions,
tests/tooling/**tests should assert observable behavior rather than incidental formatting.Source: Path instructions
tests/_helpers/apps.ts (2)
148-153: LGTM!
766-770: LGTM!Also applies to: 896-906, 1255-1261, 1561-1566, 1729-1733
ubugeeei
force-pushed
the
agent/prevent-fixture-hook-mutations
branch
from
July 12, 2026 06:02
7efdc44 to
8f79198
Compare
ubugeeei
force-pushed
the
agent/prevent-fixture-hook-mutations
branch
from
July 12, 2026 06:04
8f79198 to
ef5286f
Compare
Contributor
Detailed Test ReportCommit: Area Summary
Test InventoryTotal tracked cases: 8769 across 1265 files.
Files
Comment truncated at 64000 characters. Open the workflow run for the full job log. |
Contributor
PR BenchmarkBase:
Raw run timesCompile SFC
Lint
Format
Type check (1T)
Type check (max)
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/_helpers/apps.ts`:
- Around line 154-171: Update installPnpmDependencies to set shell: true in the
execFileSync options when invoking the Windows npx.cmd executable, while
preserving the existing behavior for non-Windows platforms and all other install
options.
In `@tests/tooling/playwright-app-config.test.ts`:
- Around line 29-32: Remove the brittle source-text assertion from the test
named “external fixture installs use the guarded helper” in
playwright-app-config.test.ts. Rely on the existing probe-based coverage to
validate the npx/pnpm@10 install invocation and safety environment variables,
without adding another source-grep replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4ba8350b-f804-43c5-89e1-90592454cb97
📒 Files selected for processing (2)
tests/_helpers/apps.tstests/tooling/playwright-app-config.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (14)
- GitHub Check: CodeRabbit
- GitHub Check: tool-benchmark
- GitHub Check: cargo-semver-checks (vize_atelier_vapor)
- GitHub Check: cargo-semver-checks (vize_atelier_sfc)
- GitHub Check: cargo-semver-checks (vize_musea)
- GitHub Check: test-scripts
- GitHub Check: test-js-packages
- GitHub Check: editor-extensions
- GitHub Check: clippy-and-test
- GitHub Check: vue-parity
- GitHub Check: build-js-packages
- GitHub Check: check-vize-apps
- GitHub Check: pr-benchmark
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (1)
tests/tooling/**
⚙️ CodeRabbit configuration file
tests/tooling/**: Ensure workflow and release tests assert observable behavior rather than incidental formatting. Prefer focused assertions that keep PRs small and failures easy to diagnose.
Files:
tests/tooling/playwright-app-config.test.ts
🪛 ast-grep (0.44.1)
tests/tooling/playwright-app-config.test.ts
[warning] 61-61: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(probeOutput, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 76-76: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(root, ".git", "config"))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 81-81: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(hooksDir, name))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 87-103: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(
probeScript,
const { spawnSync } = require("node:child_process"); const fs = require("node:fs"); const git = spawnSync("git", ["rev-parse", "--show-toplevel"], { encoding: "utf8" }); fs.writeFileSync(process.env.FIXTURE_INSTALL_PROBE, JSON.stringify({ argv: process.argv.slice(2), env: { GIT_CEILING_DIRECTORIES: process.env.GIT_CEILING_DIRECTORIES, HUSKY: process.env.HUSKY, SKIP_INSTALL_SIMPLE_GIT_HOOKS: process.env.SKIP_INSTALL_SIMPLE_GIT_HOOKS, }, gitStatus: git.status, gitTopLevel: git.stdout.trim(), })); ,
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 108-111: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(
path.join(binDir, "npx.cmd"),
@echo off\r\n"${process.execPath}" "${probeScript}" %*\r\n,
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 116-116: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(shim, #!/bin/sh\nexec "${process.execPath}" "${probeScript}" "$@"\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (4)
tests/_helpers/apps.ts (2)
148-153: LGTM!
766-769: LGTM!Also applies to: 896-905, 1255-1260, 1561-1565, 1729-1732
tests/tooling/playwright-app-config.test.ts (2)
34-72: LGTM!
74-119: LGTM!
Comment on lines
+154
to
171
| export function installPnpmDependencies(cwd: string, options: PnpmInstallOptions = {}): void { | ||
| console.log(`[vize:setup] pnpm install in ${cwd}...`); | ||
| execSync("npx -y pnpm@10 install --no-frozen-lockfile --prefer-offline", { | ||
| const args = ["-y", "pnpm@10", "install", "--no-frozen-lockfile"]; | ||
| if (options.ignoreScripts) args.push("--ignore-scripts"); | ||
| if (options.preferOffline ?? true) args.push("--prefer-offline"); | ||
| execFileSync(process.platform === "win32" ? "npx.cmd" : "npx", args, { | ||
| cwd, | ||
| env: { | ||
| ...process.env, | ||
| ...options.env, | ||
| GIT_CEILING_DIRECTORIES: path.dirname(cwd), | ||
| HUSKY: "0", | ||
| SKIP_INSTALL_SIMPLE_GIT_HOOKS: "1", | ||
| }, | ||
| stdio: "inherit", | ||
| timeout: 600_000, | ||
| timeout: options.timeout ?? 600_000, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js execFileSync .cmd file throws EINVAL without shell true CVE-2024-27980
💡 Result:
The EINVAL error you are encountering when using execFileSync (or spawn/spawnSync) to execute .bat or .cmd files on Windows is an intentional security mechanism introduced to mitigate CVE-2024-27980 [1][2][3]. CVE-2024-27980 addressed a command injection vulnerability where improper handling of batch files allowed a malicious command line argument to achieve arbitrary code execution on Windows [4][3]. To fix this, Node.js versions (starting with 18.20.2, 20.12.2, 21.7.3, and 22.0.0) now forbid spawning .bat or .cmd files directly via child_process methods unless the shell: true option is explicitly set [2][5][3]. When you attempt to run these files without shell: true on Windows, Node.js throws an EINVAL error because it no longer allows the underlying Win32 CreateProcess API to handle them directly due to the security risks associated with unescaped argument concatenation [5][6][3]. To resolve this error, you must pass the shell: true option in your function call [2][3]. Example: // Before (causes EINVAL) child_process.execFileSync('npm.cmd', ['--version']); // After child_process.execFileSync('npm.cmd', ['--version'], { shell: true }); While you can technically revert this patch by passing the --security-revert=CVE-2024-27980 flag to the Node.js process, it is strongly advised against, as it re-introduces the command injection vulnerability [3]. Additionally, be aware that in newer versions of Node.js, using shell: true with arguments may trigger a deprecation warning, as it carries its own security considerations regarding argument concatenation [7][8].
Citations:
- 1: https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2
- 2: Spawning some commands on windows fails with error EINVAL in Node 20 LTS (20.12.2) only nodejs/node#52681
- 3: https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2?hss_channel=tw-91985735
- 4: https://www.openwall.com/lists/oss-security/2024/04/10/15
- 5: nodejs/node@9095c914ed
- 6: nodejs/node@64b67779f7
- 7: Error: spawnSync npm.cmd EINVAL nodejs/node#59210
- 8: Disallow args in child_process execFile/spawn when the shell option is true nodejs/node#57143
🌐 Web query:
official Node.js docs execFileSync .cmd shell true Windows CVE-2024-27980
💡 Result:
CVE-2024-27980, often referred to as part of the BatBadBut vulnerability, is a security vulnerability in Node.js that involves improper handling of batch files (.bat and.cmd) on Windows [1][2][3]. The core of the issue is that the underlying Win32 CreateProcess API allows spawning batch files directly [4]. Node.js was historically doing this, but because this process does not correctly sanitize or escape arguments, it became possible for a malicious command-line argument to inject arbitrary commands and achieve code execution [1][4][2]. Key Technical Details: - Affected Functions: The vulnerability primarily affected child_process.spawn and child_process.spawnSync [1][4]. - Security Fix: To address this, Node.js implemented a change where attempting to directly spawn a.bat or.cmd file without the shell option enabled now results in an EINVAL error [5][4]. - Shell Option: If a developer requires the execution of a batch file, they must explicitly set the { shell: true } option [5][4]. - Scope: The vulnerability affected active release lines (18.x, 20.x, 21.x) at the time of the April 2024 security releases [1][6]. The security advisory explicitly notes that if the shell option is not enabled, the vulnerability could be triggered [1][2]. By disallowing the direct execution of these files without a shell, Node.js forces the developer to acknowledge the risks and move responsibility for argument handling to the shell environment [4]. Some subsequent discussions have explored further tightening the behavior of execFile and spawn when shell is set to true to prevent further injection patterns [7].
Citations:
- 1: https://www.openwall.com/lists/oss-security/2024/04/10/15
- 2: https://security.snyk.io/vuln/SNYK-UPSTREAM-NODE-6615824
- 3: https://nodejs.org/en/blog/vulnerability/july-2024-security-releases
- 4: nodejs/node@64b67779f7
- 5: https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2
- 6: https://soc.cyber.wa.gov.au/advisories/20240416001-Nodejs-April-Security-Release
- 7: Disallow args in child_process execFile/spawn when the shell option is true nodejs/node#57143
Add shell: true for Windows batch execution
execFileSync still throws on npx.cmd under patched Node unless shell: true is set. This helper will fail on Windows before the install starts.
Suggested fix
execFileSync(process.platform === "win32" ? "npx.cmd" : "npx", args, {
cwd,
env: {
...process.env,
...options.env,
GIT_CEILING_DIRECTORIES: path.dirname(cwd),
HUSKY: "0",
SKIP_INSTALL_SIMPLE_GIT_HOOKS: "1",
},
+ shell: process.platform === "win32",
stdio: "inherit",
timeout: options.timeout ?? 600_000,
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/_helpers/apps.ts` around lines 154 - 171, Update
installPnpmDependencies to set shell: true in the execFileSync options when
invoking the Windows npx.cmd executable, while preserving the existing behavior
for non-Windows platforms and all other install options.
Contributor
Detailed Test ReportCommit: Area Summary
Test InventoryTotal tracked cases: 8769 across 1265 files.
Files
Comment truncated at 64000 characters. Open the workflow run for the full job log. |
Contributor
Tool BenchmarkMeasured: 2026-07-12T06:10:01.835Z
Fairness notes:
Commands: gh workflow run tool-benchmark.yml --ref <branch> -f file_count=3000 -f check_file_count=500 -f vite_file_count=1000 -f nuxt_file_count=250 -f large_blocks=300 -f runs=3 -f warmups=1 -f commit_results=true
node bench/generate.mjs 3000
node bench/compare-tools.mjs --input bench/__in__ --vize-bin target/release/vize --runs 3 --warmups 1 --check-file-count 500 --vite-file-count 1000 --nuxt-file-count 250 --large-blocks 300 --runner-label "blacksmith-32vcpu-ubuntu-2404" --out tool-benchmark-summary.md --json tool-benchmark-results.json --doc performance-blacksmith.mdVariant details and raw run timesSFC compile
Large SFC compile
Large SFC type check
Lint
Format
Type check
Vite build (end-to-end)
Nuxt SPA build (end-to-end)
|
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
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
GIT_CEILING_DIRECTORIESnpxon Unix and an explicitcmd.exepath for Windows.git/config/.git/hooksinvariants at runtimeRoot cause
Exported fixture directories intentionally have no
.git. During lifecycle scripts, Git discovery therefore walked up to the Vize checkout. ELK and Reka UI run hook installers during dependency setup, which could replace the parent checkout's pre-commit hook.Impact
App E2E setup no longer mutates persistent Git state outside its fixture worktree. The centralized helper makes new exported fixtures safe by default while preserving fixture-specific flags, timeouts, and download guards.
Verification
node --test tests/tooling/playwright-app-config.test.tsvp check tests/_helpers/apps.ts tests/tooling/playwright-app-config.test.tsgit diff --checktests/_helpers/apps.tsshrinks from 2196 to 2192 linesCloses #2819
Summary by CodeRabbit
pnpm@10 installoptions (offline preference, no frozen lockfile), controlled script handling, timeouts, environment injection, and correct Windowsnpxhandling.npx -y pnpm@10 installpatterns..gitconfiguration/hooks remain unchanged and validate the probed install argv and key env values.