src: list scripts when --run has no command

[Cherry]

This extends the behaviour when you run node --run <unknown script> that prints a list of available scripts, to now print available scripts when you simply run node --run, to mimic the behaviour of npm run, etc.

$ node --runAvailable scripts are: test: node --test build: node build.js

Previously, node --run with no command errored with --run requires an argument.

This is my first contribution to Node.js so while I've tried to read and follow everything, please let me know if I'm missing anything!

Fixes: #64870

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[avivkeller]

Is there a better solution to modifying the options parser directly? We have other CLI flags that have different behavior depending on whether or not they have an argument passed?

How does --inspect do this?

[Cherry]

I'm not entirely familiar with the code, but looking at --inspect, it's a boolean option so works a bit differently I think:

node/src/node_options.cc

Lines 475 to 479 in 0bcc6ef

	AddOption("--inspect",
	"activate inspector on host:port (default: 127.0.0.1:9229)",
	&DebugOptions::inspector_enabled,
	kAllowedInEnvvar);
	AddAlias("--inspect=", { "--inspect-port", "--inspect" });

So --inspect and --inspect=1234 work, but --inspect 1234 isn't valid.

If I'm understanding correctly, --run can't do this, because it's space separated.

To avoid the hardcoded name == "--run", what would you suggest?

[avivkeller]

hmm, perhaps I was mis-remembering that --inspect required an = to run 😓. I'm still wary about changing the parser, since there probably should be a way for a flag to be passed with an optional value, regardless of =.

Sorry about misremembering, I'll ask around to see if there's a way to do this, and if not, we can always add it.

[Cherry]

thanks! It's also very possible I'm misreading this or missing something, and this is already supported somewhere - sorry if so!

[avivkeller]

I doubt that, you (so far) demonstrate a great understanding of the codebase. It's probable that we just never encountered this scenario before.

Great work so far :-)

[avivkeller]

Implementation LGTM. I'm not giving this the green check while I look into better ways of actually detecting the option-less flag, but everything from that point on is great :)

[codecov]

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (cf30b2e) to head (2958891).
⚠️ Report is 117 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options-inl.h	50.00%	0 Missing and 3 partials ⚠️
src/node_task_runner.cc	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64606 +/- ##
==========================================
+ Coverage 90.11% 90.13% +0.01% 
==========================================
Files 752 752 Lines 251861 251873 +12 Branches 47365 47363 -2 ==========================================
+ Hits 226955 227016 +61 + Misses 16238 16186 -52 - Partials 8668 8671 +3 

Files with missing lines	Coverage Δ
src/node.cc	76.53% <100.00%> (ø)
src/node_options.h	95.37% <100.00%> (+0.48%)	⬆️
src/node_task_runner.cc	89.78% <95.23%> (+0.45%)	⬆️
src/node_options-inl.h	83.09% <50.00%> (-0.18%)	⬇️

... and 32 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.

[Cherry]

Just pushed to fix the markdown linting - looks like I needed to prepend to changes, not append, sorry!

The macOS test failure looks to be an unrelated flake, but let me know if not.

[anonrig]

Thank you James! @Cherry

[Renegade334]

GHA is borked on the current PR head, and looks like there will be lint issues to address anyway, so I'm just going to trigger a rebase.

[Cherry]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

[anonrig]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

@Cherry can you rebase and ask your agent to fix the linting errors please?

[Renegade334]

The only complaint appears to be:

doc/node.1 is out of date; run `make node.1` to regenerate it.

[Cherry]

Yeah I can rebase and try and fix that shortly!

[Cherry]

Should be good now, thanks!

[nodejs-github-bot]

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

[jasnell]

Overall fine with the idea, but there's a subtly that's worth calling out.

$ export CMD=test
$ node --run $CMD
// Runs the test script
$ export CMD=
$ node --run $CMD
// Lists the available scripts

Re-using the --run could lead to edge case bugs if the user does not realize the CMD env var is not set.

[Cherry]

Yeah that's a fair point, though I think this edge-case is a pretty common shell thing - for example node $CMD with an empty var drops you into the REPL (or exits 0 in CI), and npm run $CMD / yarn / pnpm all list scripts and exit 0 the same way.

That said, since --run $CMD used to fail hard here, happy to keep the listing but exit non-zero when no command is given (like bare git) - scripts with a broken env var would still fail, and interactive users still get the list. Let me know if folks would prefer that and I'll update.

[jasnell]

That sounds like a reasonable approach to me.

[Cherry]

Updated to do a non-zero exit (9 seemed the most appropriate looking at other codes, but happy to tweak), and then print to stderrr. Let me know if there's any other feedback!

[Cherry]

CI is looking good! Let me know if there's anything further I can do to help land this. Pushed up changes to resolve the merge conflicts.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5e48fb3

[Cherry]

This extends the behaviour when you run node --run <unknown script> that prints a list of available scripts, to now print available scripts when you simply run node --run, to mimic the behaviour of npm run, etc.

$ node --runAvailable scripts are: test: node --test build: node build.js

Previously, node --run with no command errored with --run requires an argument.

This is my first contribution to Node.js so while I've tried to read and follow everything, please let me know if I'm missing anything!

Fixes: #64870

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[avivkeller]

Is there a better solution to modifying the options parser directly? We have other CLI flags that have different behavior depending on whether or not they have an argument passed?

How does --inspect do this?

[Cherry]

I'm not entirely familiar with the code, but looking at --inspect, it's a boolean option so works a bit differently I think:

node/src/node_options.cc

Lines 475 to 479 in 0bcc6ef

	AddOption("--inspect",
	"activate inspector on host:port (default: 127.0.0.1:9229)",
	&DebugOptions::inspector_enabled,
	kAllowedInEnvvar);
	AddAlias("--inspect=", { "--inspect-port", "--inspect" });

So --inspect and --inspect=1234 work, but --inspect 1234 isn't valid.

If I'm understanding correctly, --run can't do this, because it's space separated.

To avoid the hardcoded name == "--run", what would you suggest?

[avivkeller]

hmm, perhaps I was mis-remembering that --inspect required an = to run 😓. I'm still wary about changing the parser, since there probably should be a way for a flag to be passed with an optional value, regardless of =.

Sorry about misremembering, I'll ask around to see if there's a way to do this, and if not, we can always add it.

[Cherry]

thanks! It's also very possible I'm misreading this or missing something, and this is already supported somewhere - sorry if so!

[avivkeller]

I doubt that, you (so far) demonstrate a great understanding of the codebase. It's probable that we just never encountered this scenario before.

Great work so far :-)

[avivkeller]

Implementation LGTM. I'm not giving this the green check while I look into better ways of actually detecting the option-less flag, but everything from that point on is great :)

[codecov]

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (cf30b2e) to head (2958891).
⚠️ Report is 117 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options-inl.h	50.00%	0 Missing and 3 partials ⚠️
src/node_task_runner.cc	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64606 +/- ##
==========================================
+ Coverage 90.11% 90.13% +0.01% 
==========================================
Files 752 752 Lines 251861 251873 +12 Branches 47365 47363 -2 ==========================================
+ Hits 226955 227016 +61 + Misses 16238 16186 -52 - Partials 8668 8671 +3 

Files with missing lines	Coverage Δ
src/node.cc	76.53% <100.00%> (ø)
src/node_options.h	95.37% <100.00%> (+0.48%)	⬆️
src/node_task_runner.cc	89.78% <95.23%> (+0.45%)	⬆️
src/node_options-inl.h	83.09% <50.00%> (-0.18%)	⬇️

... and 32 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.

[Cherry]

Just pushed to fix the markdown linting - looks like I needed to prepend to changes, not append, sorry!

The macOS test failure looks to be an unrelated flake, but let me know if not.

[anonrig]

Thank you James! @Cherry

[Renegade334]

GHA is borked on the current PR head, and looks like there will be lint issues to address anyway, so I'm just going to trigger a rebase.

[Cherry]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

[anonrig]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

@Cherry can you rebase and ask your agent to fix the linting errors please?

[Renegade334]

The only complaint appears to be:

doc/node.1 is out of date; run `make node.1` to regenerate it.

[Cherry]

Yeah I can rebase and try and fix that shortly!

[Cherry]

Should be good now, thanks!

[nodejs-github-bot]

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

[jasnell]

Overall fine with the idea, but there's a subtly that's worth calling out.

$ export CMD=test
$ node --run $CMD
// Runs the test script
$ export CMD=
$ node --run $CMD
// Lists the available scripts

Re-using the --run could lead to edge case bugs if the user does not realize the CMD env var is not set.

[Cherry]

Yeah that's a fair point, though I think this edge-case is a pretty common shell thing - for example node $CMD with an empty var drops you into the REPL (or exits 0 in CI), and npm run $CMD / yarn / pnpm all list scripts and exit 0 the same way.

That said, since --run $CMD used to fail hard here, happy to keep the listing but exit non-zero when no command is given (like bare git) - scripts with a broken env var would still fail, and interactive users still get the list. Let me know if folks would prefer that and I'll update.

[jasnell]

That sounds like a reasonable approach to me.

[Cherry]

Updated to do a non-zero exit (9 seemed the most appropriate looking at other codes, but happy to tweak), and then print to stderrr. Let me know if there's any other feedback!

[Cherry]

CI is looking good! Let me know if there's anything further I can do to help land this. Pushed up changes to resolve the merge conflicts.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5e48fb3

[Cherry]

This extends the behaviour when you run node --run <unknown script> that prints a list of available scripts, to now print available scripts when you simply run node --run, to mimic the behaviour of npm run, etc.

$ node --runAvailable scripts are: test: node --test build: node build.js

Previously, node --run with no command errored with --run requires an argument.

This is my first contribution to Node.js so while I've tried to read and follow everything, please let me know if I'm missing anything!

Fixes: #64870

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[avivkeller]

Is there a better solution to modifying the options parser directly? We have other CLI flags that have different behavior depending on whether or not they have an argument passed?

How does --inspect do this?

[Cherry]

I'm not entirely familiar with the code, but looking at --inspect, it's a boolean option so works a bit differently I think:

node/src/node_options.cc

Lines 475 to 479 in 0bcc6ef

	AddOption("--inspect",
	"activate inspector on host:port (default: 127.0.0.1:9229)",
	&DebugOptions::inspector_enabled,
	kAllowedInEnvvar);
	AddAlias("--inspect=", { "--inspect-port", "--inspect" });

So --inspect and --inspect=1234 work, but --inspect 1234 isn't valid.

If I'm understanding correctly, --run can't do this, because it's space separated.

To avoid the hardcoded name == "--run", what would you suggest?

[avivkeller]

hmm, perhaps I was mis-remembering that --inspect required an = to run 😓. I'm still wary about changing the parser, since there probably should be a way for a flag to be passed with an optional value, regardless of =.

Sorry about misremembering, I'll ask around to see if there's a way to do this, and if not, we can always add it.

[Cherry]

thanks! It's also very possible I'm misreading this or missing something, and this is already supported somewhere - sorry if so!

[avivkeller]

I doubt that, you (so far) demonstrate a great understanding of the codebase. It's probable that we just never encountered this scenario before.

Great work so far :-)

[avivkeller]

Implementation LGTM. I'm not giving this the green check while I look into better ways of actually detecting the option-less flag, but everything from that point on is great :)

[codecov]

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (cf30b2e) to head (2958891).
⚠️ Report is 117 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options-inl.h	50.00%	0 Missing and 3 partials ⚠️
src/node_task_runner.cc	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64606 +/- ##
==========================================
+ Coverage 90.11% 90.13% +0.01% 
==========================================
Files 752 752 Lines 251861 251873 +12 Branches 47365 47363 -2 ==========================================
+ Hits 226955 227016 +61 + Misses 16238 16186 -52 - Partials 8668 8671 +3 

Files with missing lines	Coverage Δ
src/node.cc	76.53% <100.00%> (ø)
src/node_options.h	95.37% <100.00%> (+0.48%)	⬆️
src/node_task_runner.cc	89.78% <95.23%> (+0.45%)	⬆️
src/node_options-inl.h	83.09% <50.00%> (-0.18%)	⬇️

... and 32 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.

[Cherry]

Just pushed to fix the markdown linting - looks like I needed to prepend to changes, not append, sorry!

The macOS test failure looks to be an unrelated flake, but let me know if not.

[anonrig]

Thank you James! @Cherry

[Renegade334]

GHA is borked on the current PR head, and looks like there will be lint issues to address anyway, so I'm just going to trigger a rebase.

[Cherry]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

[anonrig]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

@Cherry can you rebase and ask your agent to fix the linting errors please?

[Renegade334]

The only complaint appears to be:

doc/node.1 is out of date; run `make node.1` to regenerate it.

[Cherry]

Yeah I can rebase and try and fix that shortly!

[Cherry]

Should be good now, thanks!

[nodejs-github-bot]

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

[jasnell]

Overall fine with the idea, but there's a subtly that's worth calling out.

$ export CMD=test
$ node --run $CMD
// Runs the test script
$ export CMD=
$ node --run $CMD
// Lists the available scripts

Re-using the --run could lead to edge case bugs if the user does not realize the CMD env var is not set.

[Cherry]

Yeah that's a fair point, though I think this edge-case is a pretty common shell thing - for example node $CMD with an empty var drops you into the REPL (or exits 0 in CI), and npm run $CMD / yarn / pnpm all list scripts and exit 0 the same way.

That said, since --run $CMD used to fail hard here, happy to keep the listing but exit non-zero when no command is given (like bare git) - scripts with a broken env var would still fail, and interactive users still get the list. Let me know if folks would prefer that and I'll update.

[jasnell]

That sounds like a reasonable approach to me.

[Cherry]

Updated to do a non-zero exit (9 seemed the most appropriate looking at other codes, but happy to tweak), and then print to stderrr. Let me know if there's any other feedback!

[Cherry]

CI is looking good! Let me know if there's anything further I can do to help land this. Pushed up changes to resolve the merge conflicts.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5e48fb3

[Cherry]

This extends the behaviour when you run node --run <unknown script> that prints a list of available scripts, to now print available scripts when you simply run node --run, to mimic the behaviour of npm run, etc.

$ node --runAvailable scripts are: test: node --test build: node build.js

Previously, node --run with no command errored with --run requires an argument.

This is my first contribution to Node.js so while I've tried to read and follow everything, please let me know if I'm missing anything!

Fixes: #64870

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[avivkeller]

Is there a better solution to modifying the options parser directly? We have other CLI flags that have different behavior depending on whether or not they have an argument passed?

How does --inspect do this?

[Cherry]

I'm not entirely familiar with the code, but looking at --inspect, it's a boolean option so works a bit differently I think:

node/src/node_options.cc

Lines 475 to 479 in 0bcc6ef

	AddOption("--inspect",
	"activate inspector on host:port (default: 127.0.0.1:9229)",
	&DebugOptions::inspector_enabled,
	kAllowedInEnvvar);
	AddAlias("--inspect=", { "--inspect-port", "--inspect" });

So --inspect and --inspect=1234 work, but --inspect 1234 isn't valid.

If I'm understanding correctly, --run can't do this, because it's space separated.

To avoid the hardcoded name == "--run", what would you suggest?

[avivkeller]

hmm, perhaps I was mis-remembering that --inspect required an = to run 😓. I'm still wary about changing the parser, since there probably should be a way for a flag to be passed with an optional value, regardless of =.

Sorry about misremembering, I'll ask around to see if there's a way to do this, and if not, we can always add it.

[Cherry]

thanks! It's also very possible I'm misreading this or missing something, and this is already supported somewhere - sorry if so!

[avivkeller]

I doubt that, you (so far) demonstrate a great understanding of the codebase. It's probable that we just never encountered this scenario before.

Great work so far :-)

[avivkeller]

Implementation LGTM. I'm not giving this the green check while I look into better ways of actually detecting the option-less flag, but everything from that point on is great :)

[codecov]

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (cf30b2e) to head (2958891).
⚠️ Report is 117 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options-inl.h	50.00%	0 Missing and 3 partials ⚠️
src/node_task_runner.cc	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64606 +/- ##
==========================================
+ Coverage 90.11% 90.13% +0.01% 
==========================================
Files 752 752 Lines 251861 251873 +12 Branches 47365 47363 -2 ==========================================
+ Hits 226955 227016 +61 + Misses 16238 16186 -52 - Partials 8668 8671 +3 

Files with missing lines	Coverage Δ
src/node.cc	76.53% <100.00%> (ø)
src/node_options.h	95.37% <100.00%> (+0.48%)	⬆️
src/node_task_runner.cc	89.78% <95.23%> (+0.45%)	⬆️
src/node_options-inl.h	83.09% <50.00%> (-0.18%)	⬇️

... and 32 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.

[Cherry]

Just pushed to fix the markdown linting - looks like I needed to prepend to changes, not append, sorry!

The macOS test failure looks to be an unrelated flake, but let me know if not.

[anonrig]

Thank you James! @Cherry

[Renegade334]

GHA is borked on the current PR head, and looks like there will be lint issues to address anyway, so I'm just going to trigger a rebase.

[Cherry]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

[anonrig]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

@Cherry can you rebase and ask your agent to fix the linting errors please?

[Renegade334]

The only complaint appears to be:

doc/node.1 is out of date; run `make node.1` to regenerate it.

[Cherry]

Yeah I can rebase and try and fix that shortly!

[Cherry]

Should be good now, thanks!

[nodejs-github-bot]

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

[jasnell]

Overall fine with the idea, but there's a subtly that's worth calling out.

$ export CMD=test
$ node --run $CMD
// Runs the test script
$ export CMD=
$ node --run $CMD
// Lists the available scripts

Re-using the --run could lead to edge case bugs if the user does not realize the CMD env var is not set.

[Cherry]

Yeah that's a fair point, though I think this edge-case is a pretty common shell thing - for example node $CMD with an empty var drops you into the REPL (or exits 0 in CI), and npm run $CMD / yarn / pnpm all list scripts and exit 0 the same way.

That said, since --run $CMD used to fail hard here, happy to keep the listing but exit non-zero when no command is given (like bare git) - scripts with a broken env var would still fail, and interactive users still get the list. Let me know if folks would prefer that and I'll update.

[jasnell]

That sounds like a reasonable approach to me.

[Cherry]

Updated to do a non-zero exit (9 seemed the most appropriate looking at other codes, but happy to tweak), and then print to stderrr. Let me know if there's any other feedback!

[Cherry]

CI is looking good! Let me know if there's anything further I can do to help land this. Pushed up changes to resolve the merge conflicts.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5e48fb3

[Cherry]

This extends the behaviour when you run node --run <unknown script> that prints a list of available scripts, to now print available scripts when you simply run node --run, to mimic the behaviour of npm run, etc.

$ node --runAvailable scripts are: test: node --test build: node build.js

Previously, node --run with no command errored with --run requires an argument.

This is my first contribution to Node.js so while I've tried to read and follow everything, please let me know if I'm missing anything!

Fixes: #64870

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[avivkeller]

Is there a better solution to modifying the options parser directly? We have other CLI flags that have different behavior depending on whether or not they have an argument passed?

How does --inspect do this?

[Cherry]

I'm not entirely familiar with the code, but looking at --inspect, it's a boolean option so works a bit differently I think:

node/src/node_options.cc

Lines 475 to 479 in 0bcc6ef

	AddOption("--inspect",
	"activate inspector on host:port (default: 127.0.0.1:9229)",
	&DebugOptions::inspector_enabled,
	kAllowedInEnvvar);
	AddAlias("--inspect=", { "--inspect-port", "--inspect" });

So --inspect and --inspect=1234 work, but --inspect 1234 isn't valid.

If I'm understanding correctly, --run can't do this, because it's space separated.

To avoid the hardcoded name == "--run", what would you suggest?

[avivkeller]

hmm, perhaps I was mis-remembering that --inspect required an = to run 😓. I'm still wary about changing the parser, since there probably should be a way for a flag to be passed with an optional value, regardless of =.

Sorry about misremembering, I'll ask around to see if there's a way to do this, and if not, we can always add it.

[Cherry]

thanks! It's also very possible I'm misreading this or missing something, and this is already supported somewhere - sorry if so!

[avivkeller]

I doubt that, you (so far) demonstrate a great understanding of the codebase. It's probable that we just never encountered this scenario before.

Great work so far :-)

[avivkeller]

Implementation LGTM. I'm not giving this the green check while I look into better ways of actually detecting the option-less flag, but everything from that point on is great :)

[codecov]

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (cf30b2e) to head (2958891).
⚠️ Report is 117 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options-inl.h	50.00%	0 Missing and 3 partials ⚠️
src/node_task_runner.cc	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64606 +/- ##
==========================================
+ Coverage 90.11% 90.13% +0.01% 
==========================================
Files 752 752 Lines 251861 251873 +12 Branches 47365 47363 -2 ==========================================
+ Hits 226955 227016 +61 + Misses 16238 16186 -52 - Partials 8668 8671 +3 

Files with missing lines	Coverage Δ
src/node.cc	76.53% <100.00%> (ø)
src/node_options.h	95.37% <100.00%> (+0.48%)	⬆️
src/node_task_runner.cc	89.78% <95.23%> (+0.45%)	⬆️
src/node_options-inl.h	83.09% <50.00%> (-0.18%)	⬇️

... and 32 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.

[Cherry]

Just pushed to fix the markdown linting - looks like I needed to prepend to changes, not append, sorry!

The macOS test failure looks to be an unrelated flake, but let me know if not.

[anonrig]

Thank you James! @Cherry

[Renegade334]

GHA is borked on the current PR head, and looks like there will be lint issues to address anyway, so I'm just going to trigger a rebase.

[Cherry]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

[anonrig]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

@Cherry can you rebase and ask your agent to fix the linting errors please?

[Renegade334]

The only complaint appears to be:

doc/node.1 is out of date; run `make node.1` to regenerate it.

[Cherry]

Yeah I can rebase and try and fix that shortly!

[Cherry]

Should be good now, thanks!

[nodejs-github-bot]

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

[jasnell]

Overall fine with the idea, but there's a subtly that's worth calling out.

$ export CMD=test
$ node --run $CMD
// Runs the test script
$ export CMD=
$ node --run $CMD
// Lists the available scripts

Re-using the --run could lead to edge case bugs if the user does not realize the CMD env var is not set.

[Cherry]

Yeah that's a fair point, though I think this edge-case is a pretty common shell thing - for example node $CMD with an empty var drops you into the REPL (or exits 0 in CI), and npm run $CMD / yarn / pnpm all list scripts and exit 0 the same way.

That said, since --run $CMD used to fail hard here, happy to keep the listing but exit non-zero when no command is given (like bare git) - scripts with a broken env var would still fail, and interactive users still get the list. Let me know if folks would prefer that and I'll update.

[jasnell]

That sounds like a reasonable approach to me.

[Cherry]

Updated to do a non-zero exit (9 seemed the most appropriate looking at other codes, but happy to tweak), and then print to stderrr. Let me know if there's any other feedback!

[Cherry]

CI is looking good! Let me know if there's anything further I can do to help land this. Pushed up changes to resolve the merge conflicts.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5e48fb3

[Cherry]

This extends the behaviour when you run node --run <unknown script> that prints a list of available scripts, to now print available scripts when you simply run node --run, to mimic the behaviour of npm run, etc.

$ node --runAvailable scripts are: test: node --test build: node build.js

Previously, node --run with no command errored with --run requires an argument.

This is my first contribution to Node.js so while I've tried to read and follow everything, please let me know if I'm missing anything!

Fixes: #64870

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[avivkeller]

Is there a better solution to modifying the options parser directly? We have other CLI flags that have different behavior depending on whether or not they have an argument passed?

How does --inspect do this?

[Cherry]

I'm not entirely familiar with the code, but looking at --inspect, it's a boolean option so works a bit differently I think:

node/src/node_options.cc

Lines 475 to 479 in 0bcc6ef

	AddOption("--inspect",
	"activate inspector on host:port (default: 127.0.0.1:9229)",
	&DebugOptions::inspector_enabled,
	kAllowedInEnvvar);
	AddAlias("--inspect=", { "--inspect-port", "--inspect" });

So --inspect and --inspect=1234 work, but --inspect 1234 isn't valid.

If I'm understanding correctly, --run can't do this, because it's space separated.

To avoid the hardcoded name == "--run", what would you suggest?

[avivkeller]

hmm, perhaps I was mis-remembering that --inspect required an = to run 😓. I'm still wary about changing the parser, since there probably should be a way for a flag to be passed with an optional value, regardless of =.

Sorry about misremembering, I'll ask around to see if there's a way to do this, and if not, we can always add it.

[Cherry]

thanks! It's also very possible I'm misreading this or missing something, and this is already supported somewhere - sorry if so!

[avivkeller]

I doubt that, you (so far) demonstrate a great understanding of the codebase. It's probable that we just never encountered this scenario before.

Great work so far :-)

[avivkeller]

Implementation LGTM. I'm not giving this the green check while I look into better ways of actually detecting the option-less flag, but everything from that point on is great :)

[codecov]

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (cf30b2e) to head (2958891).
⚠️ Report is 117 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options-inl.h	50.00%	0 Missing and 3 partials ⚠️
src/node_task_runner.cc	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64606 +/- ##
==========================================
+ Coverage 90.11% 90.13% +0.01% 
==========================================
Files 752 752 Lines 251861 251873 +12 Branches 47365 47363 -2 ==========================================
+ Hits 226955 227016 +61 + Misses 16238 16186 -52 - Partials 8668 8671 +3 

Files with missing lines	Coverage Δ
src/node.cc	76.53% <100.00%> (ø)
src/node_options.h	95.37% <100.00%> (+0.48%)	⬆️
src/node_task_runner.cc	89.78% <95.23%> (+0.45%)	⬆️
src/node_options-inl.h	83.09% <50.00%> (-0.18%)	⬇️

... and 32 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.

[Cherry]

Just pushed to fix the markdown linting - looks like I needed to prepend to changes, not append, sorry!

The macOS test failure looks to be an unrelated flake, but let me know if not.

[anonrig]

Thank you James! @Cherry

[Renegade334]

GHA is borked on the current PR head, and looks like there will be lint issues to address anyway, so I'm just going to trigger a rebase.

[Cherry]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

[anonrig]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

@Cherry can you rebase and ask your agent to fix the linting errors please?

[Renegade334]

The only complaint appears to be:

doc/node.1 is out of date; run `make node.1` to regenerate it.

[Cherry]

Yeah I can rebase and try and fix that shortly!

[Cherry]

Should be good now, thanks!

[nodejs-github-bot]

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

[jasnell]

Overall fine with the idea, but there's a subtly that's worth calling out.

$ export CMD=test
$ node --run $CMD
// Runs the test script
$ export CMD=
$ node --run $CMD
// Lists the available scripts

Re-using the --run could lead to edge case bugs if the user does not realize the CMD env var is not set.

[Cherry]

Yeah that's a fair point, though I think this edge-case is a pretty common shell thing - for example node $CMD with an empty var drops you into the REPL (or exits 0 in CI), and npm run $CMD / yarn / pnpm all list scripts and exit 0 the same way.

That said, since --run $CMD used to fail hard here, happy to keep the listing but exit non-zero when no command is given (like bare git) - scripts with a broken env var would still fail, and interactive users still get the list. Let me know if folks would prefer that and I'll update.

[jasnell]

That sounds like a reasonable approach to me.

[Cherry]

Updated to do a non-zero exit (9 seemed the most appropriate looking at other codes, but happy to tweak), and then print to stderrr. Let me know if there's any other feedback!

[Cherry]

CI is looking good! Let me know if there's anything further I can do to help land this. Pushed up changes to resolve the merge conflicts.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5e48fb3

[Cherry]

This extends the behaviour when you run node --run <unknown script> that prints a list of available scripts, to now print available scripts when you simply run node --run, to mimic the behaviour of npm run, etc.

$ node --runAvailable scripts are: test: node --test build: node build.js

Previously, node --run with no command errored with --run requires an argument.

This is my first contribution to Node.js so while I've tried to read and follow everything, please let me know if I'm missing anything!

Fixes: #64870

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[avivkeller]

Is there a better solution to modifying the options parser directly? We have other CLI flags that have different behavior depending on whether or not they have an argument passed?

How does --inspect do this?

[Cherry]

I'm not entirely familiar with the code, but looking at --inspect, it's a boolean option so works a bit differently I think:

node/src/node_options.cc

Lines 475 to 479 in 0bcc6ef

	AddOption("--inspect",
	"activate inspector on host:port (default: 127.0.0.1:9229)",
	&DebugOptions::inspector_enabled,
	kAllowedInEnvvar);
	AddAlias("--inspect=", { "--inspect-port", "--inspect" });

So --inspect and --inspect=1234 work, but --inspect 1234 isn't valid.

If I'm understanding correctly, --run can't do this, because it's space separated.

To avoid the hardcoded name == "--run", what would you suggest?

[avivkeller]

hmm, perhaps I was mis-remembering that --inspect required an = to run 😓. I'm still wary about changing the parser, since there probably should be a way for a flag to be passed with an optional value, regardless of =.

Sorry about misremembering, I'll ask around to see if there's a way to do this, and if not, we can always add it.

[Cherry]

thanks! It's also very possible I'm misreading this or missing something, and this is already supported somewhere - sorry if so!

[avivkeller]

I doubt that, you (so far) demonstrate a great understanding of the codebase. It's probable that we just never encountered this scenario before.

Great work so far :-)

[avivkeller]

Implementation LGTM. I'm not giving this the green check while I look into better ways of actually detecting the option-less flag, but everything from that point on is great :)

[codecov]

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (cf30b2e) to head (2958891).
⚠️ Report is 117 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options-inl.h	50.00%	0 Missing and 3 partials ⚠️
src/node_task_runner.cc	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64606 +/- ##
==========================================
+ Coverage 90.11% 90.13% +0.01% 
==========================================
Files 752 752 Lines 251861 251873 +12 Branches 47365 47363 -2 ==========================================
+ Hits 226955 227016 +61 + Misses 16238 16186 -52 - Partials 8668 8671 +3 

Files with missing lines	Coverage Δ
src/node.cc	76.53% <100.00%> (ø)
src/node_options.h	95.37% <100.00%> (+0.48%)	⬆️
src/node_task_runner.cc	89.78% <95.23%> (+0.45%)	⬆️
src/node_options-inl.h	83.09% <50.00%> (-0.18%)	⬇️

... and 32 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.

[Cherry]

Just pushed to fix the markdown linting - looks like I needed to prepend to changes, not append, sorry!

The macOS test failure looks to be an unrelated flake, but let me know if not.

[anonrig]

Thank you James! @Cherry

[Renegade334]

GHA is borked on the current PR head, and looks like there will be lint issues to address anyway, so I'm just going to trigger a rebase.

[Cherry]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

[anonrig]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

@Cherry can you rebase and ask your agent to fix the linting errors please?

[Renegade334]

The only complaint appears to be:

doc/node.1 is out of date; run `make node.1` to regenerate it.

[Cherry]

Yeah I can rebase and try and fix that shortly!

[Cherry]

Should be good now, thanks!

[nodejs-github-bot]

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

[jasnell]

Overall fine with the idea, but there's a subtly that's worth calling out.

$ export CMD=test
$ node --run $CMD
// Runs the test script
$ export CMD=
$ node --run $CMD
// Lists the available scripts

Re-using the --run could lead to edge case bugs if the user does not realize the CMD env var is not set.

[Cherry]

Yeah that's a fair point, though I think this edge-case is a pretty common shell thing - for example node $CMD with an empty var drops you into the REPL (or exits 0 in CI), and npm run $CMD / yarn / pnpm all list scripts and exit 0 the same way.

That said, since --run $CMD used to fail hard here, happy to keep the listing but exit non-zero when no command is given (like bare git) - scripts with a broken env var would still fail, and interactive users still get the list. Let me know if folks would prefer that and I'll update.

[jasnell]

That sounds like a reasonable approach to me.

[Cherry]

Updated to do a non-zero exit (9 seemed the most appropriate looking at other codes, but happy to tweak), and then print to stderrr. Let me know if there's any other feedback!

[Cherry]

CI is looking good! Let me know if there's anything further I can do to help land this. Pushed up changes to resolve the merge conflicts.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5e48fb3

[Cherry]

This extends the behaviour when you run node --run <unknown script> that prints a list of available scripts, to now print available scripts when you simply run node --run, to mimic the behaviour of npm run, etc.

$ node --runAvailable scripts are: test: node --test build: node build.js

Previously, node --run with no command errored with --run requires an argument.

This is my first contribution to Node.js so while I've tried to read and follow everything, please let me know if I'm missing anything!

Fixes: #64870

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[avivkeller]

Is there a better solution to modifying the options parser directly? We have other CLI flags that have different behavior depending on whether or not they have an argument passed?

How does --inspect do this?

[Cherry]

I'm not entirely familiar with the code, but looking at --inspect, it's a boolean option so works a bit differently I think:

node/src/node_options.cc

Lines 475 to 479 in 0bcc6ef

	AddOption("--inspect",
	"activate inspector on host:port (default: 127.0.0.1:9229)",
	&DebugOptions::inspector_enabled,
	kAllowedInEnvvar);
	AddAlias("--inspect=", { "--inspect-port", "--inspect" });

So --inspect and --inspect=1234 work, but --inspect 1234 isn't valid.

If I'm understanding correctly, --run can't do this, because it's space separated.

To avoid the hardcoded name == "--run", what would you suggest?

[avivkeller]

hmm, perhaps I was mis-remembering that --inspect required an = to run 😓. I'm still wary about changing the parser, since there probably should be a way for a flag to be passed with an optional value, regardless of =.

Sorry about misremembering, I'll ask around to see if there's a way to do this, and if not, we can always add it.

[Cherry]

thanks! It's also very possible I'm misreading this or missing something, and this is already supported somewhere - sorry if so!

[avivkeller]

I doubt that, you (so far) demonstrate a great understanding of the codebase. It's probable that we just never encountered this scenario before.

Great work so far :-)

[avivkeller]

Implementation LGTM. I'm not giving this the green check while I look into better ways of actually detecting the option-less flag, but everything from that point on is great :)

[codecov]

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (cf30b2e) to head (2958891).
⚠️ Report is 117 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options-inl.h	50.00%	0 Missing and 3 partials ⚠️
src/node_task_runner.cc	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64606 +/- ##
==========================================
+ Coverage 90.11% 90.13% +0.01% 
==========================================
Files 752 752 Lines 251861 251873 +12 Branches 47365 47363 -2 ==========================================
+ Hits 226955 227016 +61 + Misses 16238 16186 -52 - Partials 8668 8671 +3 

Files with missing lines	Coverage Δ
src/node.cc	76.53% <100.00%> (ø)
src/node_options.h	95.37% <100.00%> (+0.48%)	⬆️
src/node_task_runner.cc	89.78% <95.23%> (+0.45%)	⬆️
src/node_options-inl.h	83.09% <50.00%> (-0.18%)	⬇️

... and 32 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.

[Cherry]

Just pushed to fix the markdown linting - looks like I needed to prepend to changes, not append, sorry!

The macOS test failure looks to be an unrelated flake, but let me know if not.

[anonrig]

Thank you James! @Cherry

[Renegade334]

GHA is borked on the current PR head, and looks like there will be lint issues to address anyway, so I'm just going to trigger a rebase.

[Cherry]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

[anonrig]

Let me know if I can help with any of the linting (etc.) issues! Thanks all.

@Cherry can you rebase and ask your agent to fix the linting errors please?

[Renegade334]

The only complaint appears to be:

doc/node.1 is out of date; run `make node.1` to regenerate it.

[Cherry]

Yeah I can rebase and try and fix that shortly!

[Cherry]

Should be good now, thanks!

[nodejs-github-bot]

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

[jasnell]

Overall fine with the idea, but there's a subtly that's worth calling out.

$ export CMD=test
$ node --run $CMD
// Runs the test script
$ export CMD=
$ node --run $CMD
// Lists the available scripts

Re-using the --run could lead to edge case bugs if the user does not realize the CMD env var is not set.

[Cherry]

Yeah that's a fair point, though I think this edge-case is a pretty common shell thing - for example node $CMD with an empty var drops you into the REPL (or exits 0 in CI), and npm run $CMD / yarn / pnpm all list scripts and exit 0 the same way.

That said, since --run $CMD used to fail hard here, happy to keep the listing but exit non-zero when no command is given (like bare git) - scripts with a broken env var would still fail, and interactive users still get the list. Let me know if folks would prefer that and I'll update.

[jasnell]

That sounds like a reasonable approach to me.

[Cherry]

Updated to do a non-zero exit (9 seemed the most appropriate looking at other codes, but happy to tweak), and then print to stderrr. Let me know if there's any other feedback!

[Cherry]

CI is looking good! Let me know if there's anything further I can do to help land this. Pushed up changes to resolve the merge conflicts.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5e48fb3