fs: return Buffer from mkdtemp when prefix is a Buffer by hamidrezaghavami · Pull Request #64397 · nodejs/node

hamidrezaghavami · 2026-07-09T20:44:47Z

This PR fixes an issue where passing a Buffer as the prefix to the fs.mkdtemp family of functions incorrectly returned a string, which could mangle non-UTF8 paths.

The options encoding is now safely forced to 'buffer' when a Buffer prefix is detected for both the asynchronous, synchronous, and disposable versions.

Fixes: #58795

bakkot · 2026-07-09T21:17:35Z

Thanks!

Should be BufferIsBuffer not Buffer.isBuffer
Can't mutate the options object; you'll need to do options = { ...options, encoding: buffer } or something
Needs tests
Possibly it should be an error if encoding is explicitly specified (and not buffer)?

codecov · 2026-07-09T22:21:32Z

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (8a3b11c) to head (6bcc8a7).
⚠️ Report is 605 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/fs/promises.js	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #64397      +/-   ##
==========================================
- Coverage   90.24%   90.11%   -0.14%     
==========================================
  Files         741      741              
  Lines      241384   242121     +737     
  Branches    45480    45597     +117     
==========================================
+ Hits       217844   218183     +339     
- Misses      15097    15439     +342     
- Partials     8443     8499      +56

Files with missing lines	Coverage Δ
lib/fs.js	`98.36% <100.00%> (+<0.01%)`	⬆️
lib/internal/fs/promises.js	`92.87% <50.00%> (+0.05%)`	⬆️

... and 122 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hamidrezaghavami · 2026-07-10T12:11:51Z

@bakkot The CI is green! Here is a quick update:

Buffer check: Switched to using the internal BufferIsBuffer.
Immutability: Fixed the code to avoid mutating the user's options object.
Regression test: Added a full test suite for the Buffer prefix (test-fs-mkdtemp-buffer.js).
Original tests: Patched a string coercion bug in the original test-fs-mkdtemp.js file that was crashing the CI.

Let me know if there is anything else I need to adjust!

bakkot · 2026-07-10T16:14:50Z

-  fs.mkdtemp(Buffer.from(tmpdir.resolve('bar.')), {}, common.mustCall(handler));
-
-  // Warning fires only once
-  fs.mkdtemp(Buffer.from(tmpdir.resolve('bar.X')), common.mustCall(handler));


What happened to these tests?

bakkot · 2026-07-10T16:15:41Z

I'm not a maintainer but the new tests look good, thanks.

Still not sure about, should it be an error if encoding is explicitly specified and not buffer. I don't know if this comes up anywhere else; if so we could look at what the established pattern is. If there isn't an established pattern I would be inclined to throw here.

hamidrezaghavami · 2026-07-10T16:50:08Z

My mistake! I accidentally deleted them while fixing the string coercion bug above. Just restored them in the latest push.

hamidrezaghavami · 2026-07-10T19:21:35Z

@nodejs/collaborators All JavaScript changes and tests are perfectly clean locally!
However, I am getting a format-cpp and lint-js-and-md error from the recent upstream merge conflict on SECURITY.md and node_trace_buffer.h. Could a maintainer please help run the formatters for me since I don't have the local C++ tooling installed? (Also, test-internet appears to be a flaky failure).
Ready for your final review!

Renegade334 · 2026-07-11T07:46:43Z

Please do not add merge commits, it breaks the tooling.

https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-5-rebase

Signed-off-by: Hamid Reza Ghavami <hamidr.ghavami@gmail.com>

hamidrezaghavami · 2026-07-11T14:17:12Z

Thank you so much @aduh95 for stepping in and pushing that final fix—you are a lifesaver! 🙏 All 31 checks are completely green now! 🚀
@Renegade334 Since the CI checks have all passed, could either of you please give this a final approving review whenever you have a moment? Ready to merge!

Renegade334 · 2026-07-11T14:33:54Z

Looks like a response is outstanding to #64397 (comment)

hamidrezaghavami · 2026-07-11T15:22:00Z

@Renegade334 Thanks for pointing that out! Since bakkot mentioned he isn't a core maintainer, I wasn't sure if his question was a required change or just a general discussion, especially since @aduh95 already reviewed and force-pushed the final code today. Could you clarify if there is still anything specific you would like me to change, or are we good to go? 🙏

bakkot · 2026-07-11T15:29:36Z

@hamidrezaghavami The link goes to my comment about deleted tests, not my question about an error. You said you'd restored the deleted tests but I think that got lost in your rebase. Those tests are still missing and you should presumably put them back.

hamidrezaghavami · 2026-07-11T16:57:46Z

@bakkot Looks like @aduh95 already restored those missing tests when they force-pushed the branch earlier today! Everything is back in place now. 🙏

bakkot · 2026-07-11T17:06:56Z

No, they're still missing.

hamidrezaghavami · 2026-07-12T07:33:52Z

@Renegade334 Thank you for the review and the detailed feedback! I've updated doc/api/fs.md to align with the PR:
Updated the callback parameter and return types from {string} to {string|Buffer}.
Updated the text describing the return behaviour to explain that passing a Buffer prefix results in a Buffer path being returned.
Added the REPLACEME YAML change entries across the affected mkdtemp, mkdtempSync, and fsPromises.mkdtemp sections.
Please let me know if anything else needs tweaking! 🙏

hamidrezaghavami · 2026-07-14T09:03:27Z

Done! I've updated the descriptions for mkdtempSync and fsPromises.mkdtemp to match the same standard.
Also, it looks like a couple of the shared library CI builds failed due to unrelated infrastructure flakes. Could you, with write access, please re-run those failed jobs when you get a chance?
Thanks!

hamidrezaghavami · 2026-07-23T09:47:06Z

Done! Updated and wrapped the docs for both disposable functions. the remain error predicted its GitHub CI fasky error needs only re-run.

Renegade334

👍

hamidrezaghavami · 2026-08-10T06:52:32Z

Hey! This has the required approvals, but it looks like it got caught on some unrelated flaky CI tests. Could a maintainer please re-run the CI for me so we can get this landed?

nodejs-github-bot · 2026-08-22T02:24:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76200/

nodejs-github-bot · 2026-08-23T02:39:07Z

Commit Queue failed

   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
   ✘  semver-major requires at least 2 TSC approvals
   ✘  GitHub CI is still running

The pull request was removed from the Commit Queue and labeled commit-queue-failed An error occurred while landing this pull request using GitHub Actions. . After resolving the failure, remove that label and add commit-queue Add this label to land a pull request using GitHub Actions. to retry.

Full Commit Queue output

- Loading data for nodejs/node/pull/64397
✔  Done loading data for nodejs/node/pull/64397
----------------------------------- PR info ------------------------------------
Title      fs: return Buffer from mkdtemp when prefix is a Buffer (#64397)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     hamidrezaghavami:fix-fs-mkdtemp-buffer -> nodejs:main
Labels     fs, semver-major, author ready, needs-ci, commit-queue, commit-queue-squash
Commits    9
 - fs: return Buffer from mkdtemp when prefix is a Buffer
 - test: restore missing mkdtemp async buffer tests
 - doc: update fs.mkdtemp() return types and history
 - doc: physically wrap long lines in fs.md to fix markdown linter
 - doc: fix markdown and yaml line lengths in fs.md
 - doc: update mkdtempSync and fsPromises.mkdtemp docs
 - doc: update disposable mkdtemp return types
 - doc: wrap lines in fs.md
 - doc: add changes blocks to mkdtempDisposable functions
Committers 2
 - Antoine du Hamel <duhamelantoine1995@gmail.com>
 - Hamid Reza Ghavami <hamidr.ghavami@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/64397
Fixes: https://github.com/nodejs/node/issues/58795
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/64397
Fixes: https://github.com/nodejs/node/issues/58795
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 09 Jul 2026 20:44:47 GMT
   ✔  Approvals: 1
   ✔  - René (@Renegade334): https://github.com/nodejs/node/pull/64397#pullrequestreview-4764120032
   ✘  semver-major requires at least 2 TSC approvals
   ✘  GitHub CI is still running
   ℹ  Last Full PR CI on 2026-08-22T02:24:18Z: https://ci.nodejs.org/job/node-test-pull-request/76200/
- Querying data for job/node-test-pull-request/76200/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

View workflow run

trivikr · 2026-08-23T03:02:54Z

cc @nodejs/tsc as semver-major requires at least 2 TSC approvals

hamidrezaghavami · 2026-08-23T05:51:48Z

I have updated my public email and name settings! Could a maintainer please re-add the commit-queue label so the bot can try again?

mcollina

lgtm

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jul 9, 2026

Renegade334 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 9, 2026

hamidrezaghavami force-pushed the fix-fs-mkdtemp-buffer branch 4 times, most recently from 40f2ae2 to 3e3da50 Compare July 10, 2026 10:27

bakkot reviewed Jul 10, 2026

View reviewed changes

hamidrezaghavami force-pushed the fix-fs-mkdtemp-buffer branch from 3e3da50 to f06208f Compare July 10, 2026 16:49

hamidrezaghavami requested a review from a team as a code owner July 10, 2026 16:49

hamidrezaghavami force-pushed the fix-fs-mkdtemp-buffer branch 3 times, most recently from a3b52bb to f06208f Compare July 10, 2026 18:32

hamidrezaghavami force-pushed the fix-fs-mkdtemp-buffer branch from 546cb14 to e4ebc2b Compare July 11, 2026 09:27

fs: return Buffer from mkdtemp when prefix is a Buffer

40fba96

Signed-off-by: Hamid Reza Ghavami <hamidr.ghavami@gmail.com>

aduh95 force-pushed the fix-fs-mkdtemp-buffer branch from e4ebc2b to 40fba96 Compare July 11, 2026 11:33

aduh95 removed the request for review from a team July 11, 2026 11:35

hamidrezaghavami added 2 commits July 12, 2026 12:51

doc: physically wrap long lines in fs.md to fix markdown linter

b82da47

doc: fix markdown and yaml line lengths in fs.md

a5ef888

Renegade334 reviewed Jul 13, 2026

View reviewed changes

Comment thread doc/api/fs.md

doc: update mkdtempSync and fsPromises.mkdtemp docs

c03cea2

hamidrezaghavami force-pushed the fix-fs-mkdtemp-buffer branch from 31e7978 to c03cea2 Compare July 14, 2026 13:41

hamidrezaghavami requested a review from Renegade334 July 15, 2026 07:00

Renegade334 reviewed Jul 21, 2026

View reviewed changes

Comment thread doc/api/fs.md

hamidrezaghavami added 2 commits July 22, 2026 18:35

doc: update disposable mkdtemp return types

0d3b775

doc: wrap lines in fs.md

b7b376e

Renegade334 reviewed Jul 23, 2026

View reviewed changes

Comment thread doc/api/fs.md

doc: add changes blocks to mkdtempDisposable functions

6bcc8a7

Renegade334 approved these changes Jul 23, 2026

View reviewed changes

Renegade334 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 23, 2026

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2026

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

trivikr added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 23, 2026

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 23, 2026

trivikr removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 23, 2026

mcollina approved these changes Aug 23, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

hamidrezaghavami commented Jul 9, 2026

Uh oh!

bakkot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hamidrezaghavami commented Jul 10, 2026

Uh oh!

bakkot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

bakkot commented Jul 10, 2026

Uh oh!

hamidrezaghavami commented Jul 10, 2026

Uh oh!

hamidrezaghavami commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Renegade334 commented Jul 11, 2026

Uh oh!

hamidrezaghavami commented Jul 11, 2026

Uh oh!

Renegade334 commented Jul 11, 2026

Uh oh!

hamidrezaghavami commented Jul 11, 2026

Uh oh!

bakkot commented Jul 11, 2026

Uh oh!

hamidrezaghavami commented Jul 11, 2026

Uh oh!

bakkot commented Jul 11, 2026

Uh oh!

hamidrezaghavami commented Jul 12, 2026

Uh oh!

Uh oh!

hamidrezaghavami commented Jul 14, 2026

Uh oh!

Uh oh!

hamidrezaghavami commented Jul 23, 2026

Uh oh!

Uh oh!

Renegade334 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

hamidrezaghavami commented Aug 10, 2026

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Aug 22, 2026

Uh oh!

nodejs-github-bot commented Aug 23, 2026

Commit Queue failed

Uh oh!

trivikr commented Aug 23, 2026

Uh oh!

hamidrezaghavami commented Aug 23, 2026

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bakkot commented Jul 9, 2026 •

edited

Loading

codecov Bot commented Jul 9, 2026 •

edited

Loading

hamidrezaghavami commented Jul 10, 2026 •

edited

Loading