fix(task): run node -e/-p and node --require scripts through Deno

[divybot]

Summary

Fixes npm lifecycle scripts and deno task commands that invoke node with common flags when no real node binary is available on PATH. The task shell node command uses node_shim for parsing and translation, so node -e, node -p, node --require, and node --import run through the current Deno executable.

node -e / node -p now delegate to Deno's eval path instead of writing temporary files into node_modules. Synthetic eval modules under an npm lifecycle cwd are graph-prepared from in-memory source, so $deno$eval.* remains resolvable even when the cwd is inside an npm package. Preload emission and the legacy task-only unstable flags remain scoped to the deno task / lifecycle path so child_process behavior is unchanged.

Tests

• cargo build -p deno -p test_server
• env -u CI cargo test -p specs_tests --test specs lifecycle_scripts_node_eval -- --nocapture
• env -u CI cargo test -p specs_tests --test specs lifecycle_scripts_node_cli -- --nocapture
• cargo test -p node_shim test_translate_require --lib -- --nocapture
• cargo test -p deno task_runner --lib --features rustls/ring -- --nocapture
• ./tools/format.js --check cli/task_runner.rs cli/module_loader.rs cli/tools/run/mod.rs libs/resolver/file_fetcher.rs tests/specs/npm/lifecycle_scripts_node_eval_esm/__test__.jsonc

Closes #24916
Closes denoland/divybot#365

[bartlomieju]

Requesting changes because I'd like to do a deeper review on this one

[bartlomieju]

It feels like this is already covered by libs/node_shim/ and shouldn't be done manually?

[divybot]

Done — parse_node_args is gone; NodeCommand now uses node_shim::parse_args + node_shim::translate_to_deno_args(… for_shell_command()) for parsing and flag generation.

[bartlomieju]

Ditto, feels like this is covered by node_shim crate

[divybot]

Done — the hard-coded flag list is gone; the run/eval flags now come from node_shim::translate_to_deno_args.

[bartlomieju]

In priciple this sounds good, but I don't understand why it wouldn't work outright with deno eval - it also creates an in-memory file that is <CWD>/$deno$eval.mts

[divybot]

The in-memory part is exactly what breaks it under an npm lifecycle snapshot: <cwd>/$deno$eval.mts sits inside the package dir under node_modules, so in_npm_pkg_checker.in_npm_package() is true and resolution goes through the node/npm resolver, which can't resolve an in-memory module that isn't on disk or in the frozen snapshot (full reproduction + error in the eval-arm thread). A real on-disk file at the same path resolves fine, which is the only reason the temp .cjs works today.

[bartlomieju]

Did a deeper pass focused on the CommonJS-default question and the "isn't this node_shim's job" thread. Summary of what I found:

This duplicates libs/node_shim and there's an unused integration point made for exactly this. node_shim::translate_to_deno_args already parses -e/-p/--eval/--print/--require/-r/--import/--input-type/etc. and emits deno eval / deno run. There's even a TranslateOptions::for_shell_command() variant documented as "for transforming commands embedded in shell strings" that currently has zero callers — that looks like the slot NodeCommand was meant to plug into. So I agree with the two earlier comments: the parsing here should be node_shim, not a second hand-rolled parser.

But node_shim isn't a clean drop-in today — two gaps that are worth fixing there rather than reimplementing here:

1. It drops --require/--import: they parse into preload_cjs_modules/preload_esm_modules but translate_to_deno_args never emits them (confirmed — the only references are the struct/parse/tests). The shared node-lifecycle-scripts fixture relies on node --require ./helper.js so switching naively would regress it.
2. The -e path routes to deno eval with wrap_eval_code: false for the shell/CLI variants, so whether require works rests on deno eval's CJS auto-detection (see inline note).

On the CommonJS default specifically: the happy path is covered, but there are real divergences from Node's -e semantics and the alternative the PR rejected (deno eval) already handles more than the description implies. Details inline. Net: I'd rather see this land as (a) fix node_shim's --require/--import emission + confirm its eval-CJS, then (b) route NodeCommand through for_shell_command().

[bartlomieju]

On the CommonJS-default question and the rejected-deno eval rationale:

1. Writing .cjs only covers the CJS half of Node's -e. Node -e defaults to CommonJS but with --experimental-detect-module (default on) retries ESM-syntax code as ESM. A .cjs file is unconditionally CJS, so node -e "import x from './m.mjs'" or top-level await — both valid under Node's -e — now fail to parse. So this isn't a faithful -e; it's "-e, but always CJS."

2. The stated reasons for not using deno eval don't hold up against the code. deno eval already auto-detects CJS: eval_command sets --ext cjs when the source contains require(/module.exports/exports./__dirname/__filename (cli/tools/run/mod.rs:377), which matches the require('fs') postinstall pattern. And the eval module already resolves to <cwd>/$deno$eval.{ext} (cli/args/mod.rs:786-792), so relative require() resolves against cwd exactly like this temp file does. That undercuts both "makes relative require resolve against cwd" and the "in-memory eval module under an npm resolution snapshot" claim. If there's a genuine failure with deno eval under a snapshot, can you add a test that reproduces it? Otherwise the detect-CJS deno eval path (ideally via node_shim) looks like it covers this case without writing files into the tree.

[divybot]

You're right that deno eval already detect-CJSs and resolves the eval module at <cwd>/$deno$eval.{ext} — I started by deleting all of this and delegating fully to node_shim (so node -e → deno eval …). It works standalone, but fails specifically under an npm lifecycle-script resolution snapshot, which is the case the regression test covers.

Reproduction (NodeCommand delegating -e straight to node_shim/deno eval, the lifecycle_scripts_node_eval spec):

error: [ERR_MODULE_NOT_FOUND] Cannot find module
file:///…/node_modules/.deno/@denotest+node-eval-lifecycle-script@1.0.0/node_modules/@denotest/node-eval-lifecycle-script/.cjs
imported from file:///…/@denotest/node-eval-lifecycle-script
error: script 'postinstall' … failed with exit code 1

Root cause: a lifecycle script runs with cwd = the package dir under node_modules and NPM_RESOLUTION_STATE_FD set. The eval main module resolves to <cwd>/$deno$eval.cjs (cli/args/mod.rs:786), which in_npm_pkg_checker.in_npm_package() flags as inside an npm package, so resolution goes through the node/npm resolver — which can't find the in-memory synthetic module (it's not on disk and not in the frozen snapshot). A real file at that path is found by the fs loader, which is exactly why writing a temp .cjs and running it as a script works where deno eval doesn't.

So deno eval genuinely doesn't cover this case today; the detect-CJS and cwd-relative-resolution parts are fine, it's purely that the in-memory eval module isn't resolvable once the npm snapshot is active.

I agree the temp file is not ideal (your points about ESM--e, leak-on-SIGKILL, and read-only dirs all stand). The clean fix is to make deno eval's in-memory module resolvable under an npm snapshot, after which NodeCommand can delegate -e/-p to node_shim with no temp file and ESM -e works for free. That's a module-resolver change though — do you want me to pursue that as part of this PR, or land the node_shim parsing/translation + --require emission now and do the deno eval-under-snapshot fix as a follow-up? Happy to do either; just didn't want to land a core-resolution change without a steer.

[divybot]

Confirmed with a fresh repro on the current branch (temporarily routing node -e straight to deno eval, then running the lifecycle_scripts_node_eval spec). The exact failure:

error: script 'postinstall' in '@denotest/node-eval-lifecycle-script@1.0.0' failed with exit code 1
error: [ERR_MODULE_NOT_FOUND] Cannot find module
  '.../node_modules/.deno/@denotest+node-eval-lifecycle-script@1.0.0/node_modules/@denotest/node-eval-lifecycle-script/$deno$eval.cjs'
  imported from '.../node_modules/@denotest/node-eval-lifecycle-script'

So deno eval synthesizes $deno$eval.cjs at <cwd>, but when cwd is inside an npm package dir (which it always is for a lifecycle script), in_npm_package() is true and the node/npm resolver tries to load that synthetic specifier as a real on-disk file — which doesn't exist. A real temp .cjs at the same path resolves fine, which is the only reason the current approach works. Making deno eval work here means teaching the npm/module resolver about the in-memory eval module, which is a deeper change than this PR — happy to pursue it as a follow-up if you'd prefer that direction over the temp file.

Two honest caveats on the temp-file approach, both of which I think are acceptable for the lifecycle-script use case but want to flag explicitly:

1. It's CJS-only. The temp file is unconditionally .cjs, so ESM-syntax -e (node -e "import x from './m.mjs'", top-level await) won't parse — unlike Node's -e with --experimental-detect-module. npm lifecycle scripts in the wild are overwhelmingly node -e "...require(...)..." (the issue's motivating examples are all CJS), so I scoped to that; I can add .mjs/detection if you want ESM -e covered too.
2. It writes to cwd (leak-on-SIGKILL, read-only-dir failure), as you noted. Node's -e writes nothing.

Both are the cost of avoiding the resolver limitation above. If you'd rather I invest in the resolver fix so we can use deno eval and drop the temp file entirely, just say the word — that's the cleaner end state, just a bigger change.

[divybot]

Addressed point 1 (the CJS-only limitation) in ebec1a2: the node -e/-p temp file now picks .cjs vs .mjs using the same CJS/ESM detection deno eval uses (extracted into a shared tools::run::eval_source_is_cjs), instead of always forcing .cjs. So ESM-syntax -e (import/export, top-level await) now runs as ESM, matching Node's --experimental-detect-module. New PATH-cleared spec lifecycle_scripts_node_eval_esm runs an ESM node -e "import { writeFileSync } from 'node:fs'; …" postinstall and asserts it executes under Deno — it would fail to parse as .cjs, so it actually exercises the ESM path.

Point 2 (the deno eval route) still stands as the cleaner end state, but it's blocked on the in-memory-eval-module-under-npm-snapshot resolution issue I reproduced and traced above: eval_command stashes the source in the in-memory file_fetcher keyed by <cwd>/$deno$eval.cjs, but when cwd is inside an npm package the node/npm resolver loads the main module from disk and never consults that in-memory entry. Teaching the node resolver to honor the in-memory eval module is a core module-loader change beyond this PR's scope — happy to do it as a follow-up if you'd like that instead of the temp file, but wanted the faithful CJS+ESM behavior in place either way.

[bartlomieju]

Writing the temp .cjs into context.state.cwd() — for an npm lifecycle script that's the package dir under node_modules. Two issues: (a) on SIGKILL/panic the async move { ...; remove_file } cleanup never runs, leaking $deno$node_eval$*.cjs into the user's node_modules; (b) it fails on a read-only/locked package dir. Node's -e writes nothing to disk. This is another reason to prefer deno eval (memory file, no on-disk artifact).

[divybot]

Agreed on both — leak-on-SIGKILL and read-only/locked package dirs are real downsides of writing into cwd, and Node's -e writes nothing. This is a strong argument for the deno eval route; the blocker is the in-memory-eval-module-under-npm-snapshot resolution failure (details in the eval-arm thread). If you'd like me to fix that here, NodeCommand can drop the temp file entirely.

[bartlomieju]

Heads up if you take the "use node_shim" route the other comments suggest: node_shim currently parses --require/-r/--import into preload_cjs_modules/preload_esm_modules but translate_to_deno_args never emits them as deno run --require/--import. So you can't just delegate today without regressing the shared node --require ./helper.js fixture — the emission needs to be added to node_shim first (which is the better home for it than here).

[divybot]

Good catch — confirmed: translate_to_deno_args parsed --require/-r/--import into preload_cjs_modules/preload_esm_modules but never emitted them. I added emission to node_shim in both the run and eval translation paths (the shared home, so child_process benefits too), plus unit tests. NodeCommand carries the preloads through the -e/-p path as well. A PATH-cleared node --require ./helper.cjs main.cjs spec test now guards against the preload being dropped (asserts the preload actually ran under Deno, not a node fallback).

[divybot]

Correction to my earlier reply here: emitting --require/--import in the shared translate_to_deno_args (so child_process benefited too) was wrong, and CI caught it. Two node_compat tests spawn node -r/--import … via child_process — test-preload-self-referential.js (exec/shell path) and test-import-preload-require-cycle.js (direct spawn). Deno's preload resolution differs from Node's for some specifiers (e.g. self-referential packages), so emitting the flag there surfaced a Module not found that child_process had always silently dropped.

Fixed in 316fd8e: gated emission behind a new TranslateOptions::emit_preload_modules, enabled only by a new for_task_command() variant used by task_runner::NodeCommand. for_shell_command (child_process exec/shell rewrite) and for_child_process (direct spawn) keep their prior no-emit behavior, so the deno task/lifecycle-script feature works without changing child_process. Added a unit test asserting both child_process paths drop the preload, and both node_compat tests pass again locally.

[bartlomieju]

Good end-to-end test for node -e + require with no node on PATH. Coverage gaps for the new code paths, though:

• node -p/--print — no test; the console.log(<code>) wrapping is untested and breaks on statement (non-expression) input that Node's -p accepts.
• --require/--import — the new Run translation is only exercised by the shared node-lifecycle-scripts fixture, which runs with node on PATH, so it can't tell translation from fallback. A PATH-cleared --require test would actually prove the new path.
• ESM -e — e.g. node -e "await import('node:fs')" / top-level import; currently fails because the temp file is .cjs (see the eval-arm comment).

[divybot]

Added a new PATH-cleared lifecycle-script spec (npm/lifecycle_scripts_node_cli) covering the gaps:

• node --require ./helper.cjs main.cjs — runs with PATH="" so it can't fall back to a real node; helper.cjs records typeof Deno into a global that main.cjs reads back, so the assertion (require object) fails if the --require preload is dropped or if it didn't run under Deno. This is the test that actually distinguishes translation from fallback.
• node -p "…" — exercises the print/console.log(<code>) path, asserting it ran under Deno (print object).

ESM -e (top-level import / await import()): not covered, and it's a real limitation of the current temp-.cjs approach — the file is unconditionally CJS. As noted in the eval-arm thread, the proper fix is to delegate -e/-p to deno eval (which detect-CJS/ESMs via deno_ast), which depends on resolving the deno eval-under-npm-snapshot issue. If we go that route I'll add an ESM -e test alongside it.

[divybot]

Current head covers all three gaps from this thread:

• node -p: tests/specs/npm/lifecycle_scripts_node_cli runs node -p "..." with PATH="" and asserts the output file contains print object.
• --require: the same lifecycle_scripts_node_cli spec runs node --require ./helper.cjs main.cjs with PATH=""; helper.cjs sets a global that main.cjs records, so it fails if the preload is dropped or if the path falls back to a real node.
• ESM -e: tests/specs/npm/lifecycle_scripts_node_eval_esm runs an ESM-syntax node -e postinstall with PATH=""; node_eval_source() now picks .mjs vs .cjs using the same CJS/ESM detection as deno eval.

So this thread should be covered by the current tests.

[bartlomieju]

Opened #34637 to fix the first of the two node_shim gaps I mentioned: translate_to_deno_args was parsing --require/-r/--import into preload_cjs_modules/preload_esm_modules but never emitting them. With that landed, node_shim no longer drops preload modules, so this path can delegate to node_shim (via TranslateOptions::for_shell_command()) instead of hand-rolling a parser here. The remaining item is confirming the -e -> deno eval CJS-detection covers the require default.

[divybot]

Thanks for the deep pass and for opening #34637 — agreed on the direction. Two things:

1. node_shim --require/--import emission (#34637). I'd independently added the same emission in 412f13f (same add_preload_flags, run + eval paths, plus tests) before you opened #34637, so there's now overlap. They're equivalent — happy to drop mine and rebase onto #34637 once it lands so we don't double-maintain it (or drop it now and have this PR depend on #34637 — your call on sequencing). With node_shim emitting preloads, NodeCommand's run path already delegates to node_shim via for_shell_command() (the previously-unused variant), and I added a PATH-cleared node --require ./helper.cjs main.cjs spec that actually distinguishes translation from a node fallback.

2. The -e → deno eval blocker (answering your "add a test that reproduces it"). I tried full delegation first (-e → node_shim/deno eval); it fails under an npm lifecycle resolution snapshot. Reproduction + error are in the eval-arm thread, but the crux is a real tension:

• Node's -e resolves relative require() against cwd, and during a lifecycle script cwd is the package dir inside node_modules. deno eval's module is <cwd>/$deno$eval.cjs (cli/args/mod.rs:786), so it lands inside an npm package.
• The eval module is in-memory (insert_memory_files). When the node/npm resolver resolves a specifier inside node_modules it does a filesystem check, which misses the in-memory module → ERR_MODULE_NOT_FOUND. A real on-disk file at the same path resolves fine — which is exactly why the temp .cjs works where deno eval doesn't. (So the temp file is effectively "deno eval, but with the module written to disk so the npm resolver can find it.")

So deno eval's detect-CJS and cwd-relative resolution are both correct as you noted; the only gap is that its in-memory module isn't resolvable once the npm snapshot is active. Making it resolvable (teach the node/npm resolver to consult memory_files, or otherwise make the synthetic eval module visible) would let NodeCommand route -e/-p through for_shell_command() with no temp file and ESM -e working for free — addressing your ESM / leak / read-only points in one go.

That's a core module-resolver change though. Do you want me to take it on as part of this PR, or own it separately like #34637 (with this PR keeping the temp-file workaround until it lands)? Holding the eval path as-is pending your steer.

[divybot]

Heads-up that's relevant to #34637 — the node_shim --require/--import emission breaks a node_compat test:

---- node_compat::parallel::test-preload-self-referential.js ----
error: ifError got unwanted exception: Command failed: "${ESCAPED_0}" -r self_ref "${ESCAPED_1}"
error: Module not found "file:///.../fixtures/self_ref_module/self_ref".

That test spawns node -r self_ref <script> via child_process. With the emission, node_shim's for_child_process translation now forwards it as deno run --require self_ref <script>, and deno run --require <specifier> resolves self_ref as a path (./self_ref) rather than doing Node-style module resolution, so the self-referential package import isn't found. Node's -r self_ref resolves it as a module. So deno run --require doesn't honor bare/self-referential specifiers the way Node's -r does — emitting --require exposes that gap.

Since #34637 adds the same add_preload_flags emission, it'll hit this same test-preload-self-referential failure. Worth handling there (e.g. resolve the require specifier through node resolution before forwarding, or adjust the test) — flagging since it's the proper home for the fix.

For this PR: I reverted my duplicate emission (libs/node_shim/lib.rs is now identical to main again) to get CI green, since the emission belongs in #34637 anyway. NodeCommand still delegates parsing/translation to node_shim via for_shell_command(); --require is dropped on this branch until #34637 lands, at which point I'll rebase, pick up the emission, and re-add the PATH-cleared --require spec test. The node -e/-p temp-file path and its test are unaffected and stay green.

[bartlomieju]

Solid, well-tested fix for a real pain point (#24916): npm lifecycle scripts that shell out to node -e/-p/--require now work without a real node binary on PATH. Delegating parsing/translation to node_shim is the right direction, and the test coverage is good — PATH-cleared spec tests that actually distinguish translation from a node fallback, plus unit tests scoping preload emission away from the child_process paths.

A few things below. The unstable-flag change (inline on node_shim/lib.rs) is the one I'd most like confirmed before this lands. Not approving since CHANGES_REQUESTED is still active and the -e → resolver design question is pending your steer.

Minor / non-blocking:

• The .cjs/.mjs temp file is written into the package dir inside node_modules and cleaned up after the future completes, but a killed/interrupted process leaves it behind in the installed package. The cwd-relative-resolution rationale is sound and documented — just noting the cleanup isn't crash-safe.
• CI currently shows "No checks found" (probably the main-merge commit hasn't triggered/finished). Needs to be green before merge.

The outstanding design tension from the thread — the -e temp-file workaround existing only because deno eval's in-memory synthetic module isn't resolvable under an npm resolution snapshot — is still a maintainer call (fix the resolver here vs. split it out like #34637). Flagging so it isn't lost.

[bartlomieju]

Behavior change worth confirming: the old NodeCommand passed --unstable-bare-node-builtins, --unstable-detect-cjs, --unstable-sloppy-imports and --unstable-unsafe-proto. node_shims translation (lib.rs:3364-3366 / 3444-3446 / 3491-3493) emits --unstable-node-globals + --unstable-bare-node-builtins + --unstable-detect-cjs instead — so this drops --unstable-sloppy-imports and --unstable-unsafe-proto and adds --unstable-node-globals for every deno task/lifecycle node <script> invocation, not just the new -e/-r cases. A lifecycle script relying on sloppy imports or unsafe-proto could regress. Is dropping those intentional (matching child_process)? If so, a one-line note in the PR description (and ideally a test) would help.

[divybot]

Good catch — dropping --unstable-sloppy-imports and --unstable-unsafe-proto was unintentional, and that's a real regression for any lifecycle/deno task node <script> relying on them. Fixed in c9d3f57: added add_task_unstable_flags to TranslateOptions, enabled only for for_task_command(), which emits those two flags on top of the standard trio at all three translation sites. for_child_process()/for_shell_command() stay unchanged (they never passed them). New unit test test_task_command_emits_legacy_unstable_flags asserts the task path emits both and the child_process/shell paths do not; updated the exact-match assertion in test_translate_require_and_import_for_script.

Adding --unstable-node-globals to the task path I kept intentionally — it aligns the deno task node shim with what child_process already passes and is compat-additive (exposes Node globals), so it shouldn't regress anything.

[bartlomieju]

Minor: reconstructing run_args from only --require/--import + tempfile + eval_argv drops any other flags that were on the original node -e invocation. e.g. node --no-warnings -e "..." loses --no-warnings (and v8 flags / --conditions), because the reparse discards them before the NODE_OPTIONS forwarding below ever sees them. Probably an acceptable edge case, but worth a comment noting the limitation — or carrying node_options/v8 flags through the reparse.

[divybot]

Carried them through in c9d3f57. Instead of reparsing a reconstructed --require/--import + tempfile + eval_argv list (which discarded --no-warnings, V8 flags, --conditions, etc.), the -e/-p handling now mutates the already-parsed result in place: it clears the eval markers (has_eval_string/eval_string/print_eval) and sets remaining_args = [tempfile, ...eval_argv], leaving every other parsed flag intact. The run-path translation then emits V8 flags + --conditions (via add_common_flags), --no-warnings→NODE_OPTIONS, and the preloads (via add_preload_modules) as before. So node --no-warnings --conditions=foo -e "..." now keeps all of those. Bonus: it also drops the now-impossible reparse error/fallback branch.

[divybot]

Thanks for the deep pass. Addressed both inline points in c9d3f57:

• Unstable flags: restored --unstable-sloppy-imports + --unstable-unsafe-proto for the deno task/lifecycle node <script> path via a for_task_command()-only add_task_unstable_flags option, so no regression vs. the old hand-rolled NodeCommand. Kept --unstable-node-globals (compat-additive, aligns with child_process). Unit tests added/updated.
• node -e/-p flag loss: now mutates the parsed result in place rather than reparsing a minimal arg list, so --no-warnings, V8 flags, --conditions, and preloads all carry through.

Existing specs still green locally (lifecycle_scripts_node_eval, _eval_esm, _cli) plus the new node_shim unit tests.

On the two items I left as-is:

• Temp-file crash-safety: agreed it's not crash-safe on SIGKILL; left documented as-is since it's tied to the cwd-relative-resolution workaround. Happy to revisit if you'd prefer a different cleanup strategy.
• -e → in-memory resolver vs. temp file: still flagging this as the maintainer call it is (fix the npm-snapshot resolution of deno eval's synthetic module here, or split it out like fix(node_shim): forward --require/--import to translated deno args #34637). The temp-file route is the workaround until that's decided.

[divybot]

Status after the latest review update:

• No new code changes needed on top of c9d3f57 from what I can see. The two later inline comments are already addressed on the current head: task-only unstable flags are restored via TranslateOptions::for_task_command(), and node -e/-p now mutates the parsed node_shim::ParseResult in place so --no-warnings, V8 flags, conditions, and preloads carry through.
• fix(node_shim): forward --require/--import to translated deno args #34637 is still open, not merged into main, so this PR cannot currently rebase onto it as a dependency. This branch keeps the scoped task-path preload emission to avoid changing child_process behavior.
• The remaining design point is still the maintainer call called out in the review: the temp-file workaround exists only because deno eval's in-memory $deno$eval.* module is not resolvable under the npm lifecycle resolution snapshot. I can split that resolver fix out or replace the temp-file path here once that direction is chosen.

No push from me this round; branch is already up to date and local focused tests/format were green.

[divybot]

Pushed a follow-up that removes the task-runner temp-file eval path.

node -e / node -p now go through the same node_shim translation as the other supported node CLI flags and delegate to deno eval. The missing piece was that lifecycle scripts can run with a cwd inside node_modules, so $deno$eval.* specifiers were being treated as physical npm package files before the module loader checked MemoryFiles. The new change lets memory-backed specifiers under npm package cwd enter graph preparation, which keeps eval source in memory and avoids writing helper files into packages.

Focused checks passed locally:

• ./x test-spec npm/lifecycle_scripts_node_eval npm/lifecycle_scripts_node_eval_esm npm/lifecycle_scripts_node_cli
• cargo test -p node_shim test_translate_require --lib -- --nocapture
• cargo test -p deno task_runner --lib --features rustls/ring -- --nocapture
• ./tools/format.js --check cli/task_runner.rs cli/module_loader.rs cli/tools/run/mod.rs tests/specs/npm/lifecycle_scripts_node_eval_esm/__test__.jsonc

[divybot]

Pushed 1239ccc513 to address the debug spec failures from 769b0c3b.

Root cause: prepare_load saw the $deno$eval.* source in memory, but DenoGraphLoader::load checked /node_modules/ externalization before file_content_overrides, so the prepared graph still treated the eval URL as an external npm package file. Runtime then fell through to NpmModuleLoader and tried to read $deno$eval.cjs / $deno$eval.mts from disk.

Fix:

• cli/module_loader.rs now passes maybe_code / MemoryFiles source as an explicit graph file override for prepared loads.
• libs/resolver/file_fetcher.rs now lets explicit file_content_overrides win before the node_modules externalization branch.
• Added a resolver loader unit guard for file overrides under node_modules.

Checks run locally:

• cargo build -p deno -p test_server
• env -u CI cargo test -p specs_tests --test specs lifecycle_scripts_node_eval -- --nocapture
• env -u CI cargo test -p specs_tests --test specs lifecycle_scripts_node_cli -- --nocapture
• cargo test -p node_shim test_translate_require --lib -- --nocapture
• cargo test -p deno task_runner --lib --features rustls/ring -- --nocapture
• ./tools/format.js --check cli/task_runner.rs cli/module_loader.rs cli/tools/run/mod.rs libs/resolver/file_fetcher.rs tests/specs/npm/lifecycle_scripts_node_eval_esm/__test__.jsonc

Note: I also tried cargo test -p deno_resolver file_content_overrides_load_returns_node_modules_content --lib -- --nocapture, but this worktree currently hits unrelated existing RealSys: SystemRandom test compile errors in libs/resolver/cache/* before it can run that focused test.

[bartlomieju]

Nicely structured and very well commented. A few behavior-change and robustness points worth confirming:

1. Flag-first invocations no longer defer to a real node. The old NodeCommand deferred to the node binary whenever args[0] started with -. Now everything is parsed by node_shim and only the parse_args error / REPL cases defer. So node -e/-p run under Deno even when a real node is on PATH, and node --version / --help / --check / --inspect-brk etc. depend entirely on whether parse_args rejects them (→ fallback) or translates them (→ output differs from real node). Can we confirm parse_args actually errors on the flags we mean to leave to real node, and add a test for the unsupported-flag fallback contract plus node --version?

2. node -e CJS/ESM detection diverges from Node. Node's -e defaults to CommonJS; here it goes through eval_source_is_cjs (ESM unless CJS substrings are found). The detection is substring-based, so node -e "console.log('require(')" is misclassified as CJS, and code with no hints runs as ESM (top-level this/require/__dirname then differ from Node). wrap_eval_code is also false on the task path, so the Node-eval global wrapping that child_process gets is absent. Reasonable pragmatic tradeoff, but worth calling out explicitly as a known divergence so it isn't mistaken for full node -e fidelity.

3. file_fetcher.rs reorder changes precedence for all callers. Moving the file_content_overrides check to the top of load() makes overrides win over the /node_modules/ path rewriting and the prior early-return branch. That's what makes $deno$eval.* resolvable (and the new unit test covers it), but the effect is broader than the eval fix strictly needs - overrides now bypass node_modules normalization for every caller. Intended?

4. Windows + "PATH": "" in the spec tests. An empty PATH can interact oddly with process spawning / DLL lookup on Windows. Are these CI-portable on Windows, or do they need an OS guard? (Deno itself is launched via current_exe(), so that part is fine.)

Minor:

• pub fn eval_source_is_cjs - does it need to be pub? Only referenced within run/mod.rs in this diff.
• The three add_task_unstable_flags emission blocks in translate_to_deno_args are copy-pasted across the eval/print/script branches; a small helper (like add_preload_modules) would keep them from drifting.

The module_loader.rs changes look appropriately narrow (short-circuit only fires for in-npm-package memory-file specifiers), and the for_task_command vs for_child_process/for_shell_command scoping with the regression tests for the self-referential-preload case is a nice guard.

[divybot]

Pushed ac9e0b1a75 addressing the latest review pass.\n\n- Preserved the old real-node fallback for flag-first invocations except the task/lifecycle shapes this PR intentionally shims: node -e/-p, preloads with a script, --test, and --run. Added focused task_runner unit tests covering fallback for node --version, node --check script.js, inspector/script flags, and unknown flag-only commands, plus the supported translated shapes.\n- Kept node --version out of the Deno translation path by the same fallback predicate.\n- Made the node -e fidelity tradeoff explicit in TranslateOptions::for_task_command(): this uses Deno eval CJS detection for lifecycle compatibility, not exact Node CLI semantics.\n- Documented that graph file overrides intentionally win before npm externalization so in-memory .* modules under node_modules resolve.\n- Made eval_source_is_cjs private and factored the repeated task unstable flag emission into a helper.\n\nChecks run locally:\n\n- cargo test -p node_shim test_translate_require --lib -- --nocapture\n- cargo test -p deno task_runner --lib --features rustls/ring -- --nocapture\n- ./x test-spec npm/lifecycle_scripts_node_eval npm/lifecycle_scripts_node_eval_esm npm/lifecycle_scripts_node_cli\n- ./tools/format.js --check cli/task_runner.rs cli/tools/run/mod.rs libs/node_shim/lib.rs libs/resolver/file_fetcher.rs tests/specs/npm/lifecycle_scripts_node_eval/__test__.jsonc tests/specs/npm/lifecycle_scripts_node_eval_esm/__test__.jsonc tests/specs/npm/lifecycle_scripts_node_cli/__test__.jsonc