Review requested:

• @nodejs/startup

Thanks for taking this one on! ❤️

A few things though:

Why would mutating the error be preferable to using error.cause? (For example:

This also needs a test. I think the change to the native function may make the javascript change unreachable due to no longer having the syscall info. 🤔

And lastly, a more minor detail: a couple lint checks fail as the commit message is too long and the message lines are too long. We limit lines to 80 columns.

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.11%. Comparing base (cc7018e) to head (6599712).
Report is 28 commits behind head on main.

@@           Coverage Diff           @@
##             main   #57053   +/-   ##
=======================================
  Coverage   89.10%   89.11%           
=======================================
  Files         665      665           
  Lines      193203   193213   +10     
  Branches    37216    37216           
=======================================
+ Hits       172158   172181   +23     
- Misses      13768    13778   +10     
+ Partials     7277     7254   -23     

what should further with this pr

Why would mutating the error be preferable to using error.cause?

I agree that error.cause is better!

I believe originally there was a new error thrown without .cause and I had said that it would be better to throw the same error instead, but error.cause would contain even more information so I think it's the best way to do it

what should further with this pr

I think @jasnell has the best advice here--add the error as a THROW_ERR_... macro in node_errors.h and just do everything on the native side. No need for a JS side wrapping error at all if the original error from the native side has sufficient information included.

Please check the pr
I have made the necessary changes

Code change LGTM but this needs a test. The test might be a bit tricky but it could create a temporary directory with a script in it, run that script, delete that directory, then call the api from the script that was in that directory, etc.... anyway, once a test is added I'm happy to approve this.

Code change LGTM but this needs a test. The test might be a bit tricky but it could create a temporary directory with a script in it, run that script, delete that directory, then call the api from the script that was in that directory, etc.... anyway, once a test is added I'm happy to approve this.

I am sorry but i am not able to do it.

@addaleax can you check now please

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

Now what should I do

There's a merge conflict that needs to be resolved. Rebase your branch on the latest main and resolve the conflict then we'll be able to move forward.

@jasnell I have done the merging of conflicts can you tell if something more to be done

I there still some problem?

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

No idea why that CI run didn't start. Trying again.

@Ankush1oo8 ... here's a possible test that works locally for me. We'll need to verify that it works on all platforms.

'use strict';

// This test verifies that an appropriate error is thrown when the current
// working directory is deleted and process.cwd() is called.
//
// We do this by spawning a forked process and deleting the tmp cwd
// while it is starting up.

const common = require('../common');

const { fork } = require('node:child_process');
const { rmSync } = require('node:fs');
const { strictEqual, ok } = require('node:assert');
const { Buffer } = require('node:buffer');

if (process.argv[2] === 'child') {
  return;
}

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();
const tmpDir = tmpdir.path;

const proc = fork(__filename, ['child'], {
  cwd: tmpDir,
  silent: true,
});
proc.on('spawn', common.mustCall(() => rmSync(tmpDir, { recursive: true })));

proc.on('exit', common.mustCall((code) => {
  strictEqual(code, 1);
  proc.stderr.toArray().then(common.mustCall((chunks) => {
    const buf = Buffer.concat(chunks);
    ok(/Current working directory does not exist/.test(buf.toString()));
  }));
}));

@jasnell Okey

How do we that for all platforms

Just add the test in test/parallel and CI will run it on the various platforms.

Okey

@jasnell can you check now?

@Ankush1oo8 ... unfortunately it looks like you used a merge commit to catch this up to main. We don't do merge commits. Can I ask you to remove the merge commit and use rebase to catch it up?

@jasnell Shall I delete this PR and again send one

No need. You ought to be able to reset your local branch to just your commits to drop the merge commit.