test(scripts): cover repository-linter race guards - #827
Conversation
|
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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}
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 `@scripts/__tests__/lint-no-hardcoded-runners.test.mjs`:
- Around line 15-18: Update the runScript helper to use an API such as spawnSync
that captures both stdout and stderr on successful execution, then combine those
streams into output before returning it. Preserve the existing exitCode behavior
and ensure assertions can inspect zero-exit stderr output.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e483b7d-75cf-429c-b53d-884a7bf605b3
📒 Files selected for processing (2)
scripts/__tests__/lint-no-hardcoded-runners.test.mjsscripts/__tests__/vitest-config.test.mjs
| const output = execFileSync(process.execPath, [script, ...targets], { | ||
| encoding: 'utf8', | ||
| }) | ||
| return { exitCode: 0, output } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching target:'
fd -a 'lint-no-hardcoded-runners.test.mjs' . || true
file="$(fd 'lint-no-hardcoded-runners.test.mjs' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
printf '\nFile: %s\n' "$file"
wc -l "$file"
printf '\nRelevant test file excerpts:\n'
sed -n '1,120p' "$file" | cat -n
fi
printf '\nExecutable files under scripts that might be test targets:\n'
find scripts -maxden depth 3 -type f -executable 2>/dev/null | sed -n '1,120p' || true
printf '\nSearch for spawnSync/fetch/execFileSync in test file:\n'
rg -n "execFileSync|spawnSync|stderr|stdout|exitCode|diagnostic|error" "$file" 2>/dev/null || trueRepository: cipherstash/stack
Length of output: 6398
🌐 Web query:
Node.js child_process execFileSync return value stdout on success does not return stderr documentation
💡 Result:
The child_process.execFileSync method in Node.js returns only the stdout of the command as a string or Buffer upon success [1][2][3]. It does not return stderr, nor does it return an object containing both stdout and stderr [1][4]. By default, the stderr of the child process is piped to the parent process's stderr [1][5]. If you need to capture stderr, you must explicitly configure the stdio option to pipe it [1][6]. However, even when stdio is configured to 'pipe', the return value of execFileSync remains only the stdout [1][2]. To access stderr when using execFileSync, you must handle the error thrown if the process exits with a non-zero exit code [1][7]. When the process fails, the thrown Error object includes the stdout and stderr properties [1][4][7]. If the process succeeds, however, stderr is typically inherited by the parent process and is not available via the return value of the function [1][5]. To capture both streams successfully during normal execution, you may need to use different child process methods or manually manage the stdio streams [8].
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://beta.docs.nodejs.org/child_process.html
- 3: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/122c5336ff41ad8b0e3ebddc4e96e34facaaf8e5/types/node/child_process.d.ts
- 4: https://github.com/nodejs/node/blob/d90d9d55/doc/api/child_process.md
- 5: https://nodejs.org/docs/latest-v24.x/api/child_process.html
- 6: https://nodejs.org/api/child_process.md
- 7: https://r2.nodejs.org/dist/v22.5.0/docs/api/child_process.html
- 8: https://stackoverflow.com/questions/73387244/node-js-execfilesync-execfile-merge-a-stdout-and-stderr
Capture stderr on successful runs too.
execFileSync() returns only stdout on success, so zero-exit stderr output is unavailable to the runScript() helper. Use an API that exposes both streams, such as spawnSync, and combine them before the assertions check output.
🤖 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 `@scripts/__tests__/lint-no-hardcoded-runners.test.mjs` around lines 15 - 18,
Update the runScript helper to use an API such as spawnSync that captures both
stdout and stderr on successful execution, then combine those streams into
output before returning it. Preserve the existing exitCode behavior and ensure
assertions can inspect zero-exit stderr output.
Inline Feedback AddressedVerified and fixed the successful-stderr capture finding in
Commit: Validation passed: 137 scripts tests, Biome, and a zero-finding CodeRabbit review. |
2 participants
Summary
walk()ENOENT skip path and unexpected-error rethrow pathfileParallelism: falsescripts-test settingFollow-up to #824, addressing its unresolved review feedback.
Validation
pnpm test:scripts— 137 tests passedNo changeset: test-only repository tooling changes.