fix: Pipe process.stdout.write through Buffer to fix node incompatibility#29232
fix: Pipe process.stdout.write through Buffer to fix node incompatibility#29232lindskogen wants to merge 1 commit intooven-sh:mainfrom
Conversation
WalkthroughThe changes add input normalization for string encoding handling in file stream write operations and introduce comprehensive regression tests for stdout write operations with various encodings. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/process/process-stdio.test.ts`:
- Around line 164-204: Each test that spawns a process (the three tests using
spawnSync with bunExe and bunEnv) lacks an assertion on the child process exit
code; update each test to capture the spawnSync result's exitCode (from the
object returned by spawnSync) and add an assertion after the existing stdout
Buffer checks to ensure exitCode is 0 (or the expected success code). Locate the
three tests named "process.stdout.write(string, '%s') writes raw bytes",
"process.stdout.write(string, 'hex') decodes hex", and
"process.stdout.write(string) defaults to UTF-8" and add the exitCode assertion
immediately after the Buffer.compare / stdout checks for each spawnSync call.
🪄 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
Run ID: 4401ef1f-f0f0-40a2-9359-e7472a07009e
📒 Files selected for processing (2)
src/js/internal/fs/streams.tstest/js/node/process/process-stdio.test.ts
| test.each(["binary", "latin1"] as const)("process.stdout.write(string, '%s') writes raw bytes", encoding => { | ||
| const { stdout } = spawnSync({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| `for (let i = 0; i <= 0xff; i++) process.stdout.write(String.fromCharCode(i), ${JSON.stringify(encoding)});`, | ||
| ], | ||
| stdout: "pipe", | ||
| stdin: null, | ||
| stderr: "inherit", | ||
| env: bunEnv, | ||
| }); | ||
| expect(stdout).toBeInstanceOf(Buffer); | ||
| expect(stdout.length).toBe(256); | ||
| const expected = Buffer.alloc(256); | ||
| for (let i = 0; i < 256; i++) expected[i] = i; | ||
| expect(Buffer.compare(stdout, expected)).toBe(0); | ||
| }); | ||
|
|
||
| test("process.stdout.write(string, 'hex') decodes hex", () => { | ||
| const { stdout } = spawnSync({ | ||
| cmd: [bunExe(), "-e", `process.stdout.write("deadbeef", "hex");`], | ||
| stdout: "pipe", | ||
| stdin: null, | ||
| stderr: "inherit", | ||
| env: bunEnv, | ||
| }); | ||
| expect(Buffer.compare(stdout, Buffer.from([0xde, 0xad, 0xbe, 0xef]))).toBe(0); | ||
| }); | ||
|
|
||
| test("process.stdout.write(string) defaults to UTF-8", () => { | ||
| const { stdout } = spawnSync({ | ||
| cmd: [bunExe(), "-e", `process.stdout.write("héllo");`], | ||
| stdout: "pipe", | ||
| stdin: null, | ||
| stderr: "inherit", | ||
| env: bunEnv, | ||
| }); | ||
| // é = U+00E9 = UTF-8 c3 a9 | ||
| expect(Buffer.compare(stdout, Buffer.from([0x68, 0xc3, 0xa9, 0x6c, 0x6c, 0x6f]))).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Add exit-code assertions to each new spawnSync regression test.
Each new test validates stdout bytes but never asserts process success. Please capture exitCode and assert it after stdout checks in all three tests.
Suggested patch
- const { stdout } = spawnSync({
+ const { stdout, exitCode } = spawnSync({
cmd: [
bunExe(),
"-e",
`for (let i = 0; i <= 0xff; i++) process.stdout.write(String.fromCharCode(i), ${JSON.stringify(encoding)});`,
],
@@
expect(stdout.length).toBe(256);
const expected = Buffer.alloc(256);
for (let i = 0; i < 256; i++) expected[i] = i;
expect(Buffer.compare(stdout, expected)).toBe(0);
+ expect(exitCode).toBe(0);
});
test("process.stdout.write(string, 'hex') decodes hex", () => {
- const { stdout } = spawnSync({
+ const { stdout, exitCode } = spawnSync({
cmd: [bunExe(), "-e", `process.stdout.write("deadbeef", "hex");`],
stdout: "pipe",
stdin: null,
stderr: "inherit",
env: bunEnv,
});
expect(Buffer.compare(stdout, Buffer.from([0xde, 0xad, 0xbe, 0xef]))).toBe(0);
+ expect(exitCode).toBe(0);
});
test("process.stdout.write(string) defaults to UTF-8", () => {
- const { stdout } = spawnSync({
+ const { stdout, exitCode } = spawnSync({
cmd: [bunExe(), "-e", `process.stdout.write("héllo");`],
stdout: "pipe",
stdin: null,
stderr: "inherit",
env: bunEnv,
});
// é = U+00E9 = UTF-8 c3 a9
expect(Buffer.compare(stdout, Buffer.from([0x68, 0xc3, 0xa9, 0x6c, 0x6c, 0x6f]))).toBe(0);
+ expect(exitCode).toBe(0);
});As per coding guidelines, “Always check exit codes and test error scenarios when spawning processes in tests” and “Expect stdout assertions before exit code assertions.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/process/process-stdio.test.ts` around lines 164 - 204, Each test
that spawns a process (the three tests using spawnSync with bunExe and bunEnv)
lacks an assertion on the child process exit code; update each test to capture
the spawnSync result's exitCode (from the object returned by spawnSync) and add
an assertion after the existing stdout Buffer checks to ensure exitCode is 0 (or
the expected success code). Locate the three tests named
"process.stdout.write(string, '%s') writes raw bytes",
"process.stdout.write(string, 'hex') decodes hex", and
"process.stdout.write(string) defaults to UTF-8" and add the exitCode assertion
immediately after the Buffer.compare / stdout checks for each spawnSync call.
What does this PR do?
Happened upon a bug in bun, where it currently ignores the
encodingparameter ofprocess.stdout.write. This PR makes it so it usesBuffer.fromto encode the data.How did you verify your code works?