fix(ext/process): tolerate unlinked cwd and accept top-level --interactive Node compat flag#33587
fix(ext/process): tolerate unlinked cwd and accept top-level --interactive Node compat flag#33587divybot wants to merge 5 commits intodenoland:mainfrom
Conversation
…ctive Node compat flag Enables tests/node_compat/runner/suite/test/parallel/test-cwd-enoent-repl.js Co-authored-by: Divy Srivastava <me@littledivy.com>
| .arg( | ||
| // Node.js compatibility: `node --interactive` / `node -i` forces REPL | ||
| // mode. We accept it as a no-op flag at the top level so that scripts | ||
| // spawning the binary the same way fall through to Deno's default | ||
| // REPL behavior. Deliberately not global() — only valid when no | ||
| // subcommand is given. | ||
| Arg::new("interactive") | ||
| .short('i') | ||
| .long("interactive") | ||
| .help("Enter REPL mode (Node.js compatibility)") | ||
| .action(ArgAction::SetTrue) | ||
| .hide(true), | ||
| ) |
There was a problem hiding this comment.
I'm not sure about this one... if it's needed for tests then we should improve our shim wrapper to handle that correctly (libs/node_shim/)
There was a problem hiding this comment.
The shim wrapper (libs/node_shim/) only sees the args when the user (or a child spawn) invokes the node_shim binary directly. The Node-compat test that motivates this PR (tests/node_compat/runner/suite/test/parallel/test-cwd-enoent-repl.js) does:
const child = spawn(process.execPath, ['-i']);process.execPath inside Deno is Deno.execPath() — the deno binary, not node_shim. So the args reach Deno's clap parser unmediated. Routing through the shim would mean either (a) shipping the shim alongside deno and pointing process.execPath at it in node-compat mode, or (b) having Deno embed node_shim::parse_args as a pre-clap fallback for unknown args. Both are bigger architectural moves than this PR; happy to do them in a follow-up if you'd prefer.
For this PR I kept the flag minimal: hidden, top-level only (not .global()), and the parsing branch only forces REPL when no other meaningful args are present. That limits the surface area to what the test (and any analogous Node-mimicking spawn) actually exercises. WDYT — keep it as-is and move the shim integration to a follow-up, or block this on doing the shim integration first?
| /// `None` means "inherit the parent's cwd". This happens when no cwd was | ||
| /// explicitly passed and the current working directory could not be | ||
| /// resolved (e.g. because it was unlinked, matching Node.js semantics). | ||
| cwd: Option<PathBuf>, |
There was a problem hiding this comment.
I don't like this change... SpawnArgs.cwd is already an option. We should just compute it before creating RunEnv instead of passing this option all the way down
There was a problem hiding this comment.
Addressed in e0df4fb:
RunEnv.cwdis nowPathBufagain. The inherit-parent-cwd decision is computed once insidecompute_run_envand stored in a separateset_cwd_on_command: boolfield. Nothing downstream ofRunEnvdeals with anOption<PathBuf>anymore —resolve_cmdandwhich_insee a concretePathBuf(set to"."as a placeholder when the cwd is unknown).- Inheritance is also scoped:
compute_run_envtakes anallow_cwd_inherit: boolargument that onlyop_node_spawn_childpasses astrue.op_spawn_child,op_spawn_sync, andop_runstill error withfailed resolving cwd:when the parent's cwd is unlinked, which restores thetests/unit/process_test.ts:581 nonExistentCwdcontract that the unit-test CI was failing on.
fibibot
left a comment
There was a problem hiding this comment.
Walking back the prior APPROVE — I shouldn't have signed off while CI was still building. test unit debug macos-aarch64 finished red shortly after my review, and the failure is directly caused by this PR's behavior change.
tests/unit/process_test.ts:581 nonExistentCwd explicitly asserts that spawning a child after chdir(tempDir); removeSync(tempDir); fails with exit code 1 and stderr "failed resolving cwd:". The PR's cwd: Option<PathBuf> change (making compute_run_env fall back to current_dir().ok() when no arg_cwd is given) flips that to success — the test now sees code === 0 instead of 1:
nonExistentCwd => ./tests/unit/process_test.ts:581:6
error: AssertionError: Values are not strictly equal.
- 0
+ 1
Two reasonable resolutions:
-
Update the test to match the new behavior. Since the goal is Node-compat (Node allows this — that's what
test-cwd-enoent-replexercises), changeassertStrictEquals(code, 1)→assertStrictEquals(code, 0)and drop thefailed resolving cwd:stderr assertion. The test name should probably also be updated to reflect that the new contract is "spawn from a deleted cwd succeeds via inherited cwd." -
Scope the cwd-inherit behavior more narrowly. Only fall back to
Nonewhen the spawn is from a Node-compat code path (e.g.child_process.spawn/fork), not when it's fromDeno.run/Deno.Command. That would keep the existing Deno-API contract while still satisfyingtest-cwd-enoent-repl. This is more invasive but preserves an existing user-facing guarantee.
Worth picking deliberately rather than just flipping the assertion — there may be users relying on the "spawn fails when cwd is gone" signal as an early-error indicator.
Also flagging this as a change that should get a maintainer's eyes — it adds a new top-level CLI flag (--interactive / -i) and changes process-spawn semantics. cc @bartlomieju.
…ode-compat Addresses review on denoland#33587: - bartlomieju: keep RunEnv.cwd as PathBuf instead of Option<PathBuf>; the inherit-parent-cwd decision is now a separate set_cwd_on_command bool computed once in compute_run_env. The Option no longer propagates through resolve_cmd / which_in. - fibibot (CHANGES_REQUESTED, Option 2): scope cwd-inheritance to op_node_spawn_child only. Deno.run / Deno.Command / Deno.Command.outputSync still error with "failed resolving cwd:" when the parent cwd is unlinked, restoring the existing tests/unit/process_test.ts:581 nonExistentCwd contract. Only Node child_process.spawn opts in via allow_cwd_inherit=true. - fibibot (non-blocking inline): added a comment on the which_in lookup_cwd "." fallback noting it is best-effort when the parent cwd is unlinked. Verified: tests/node_compat/parallel/test-cwd-enoent-repl.js still passes; tests/unit/process_test.ts (process_test, including nonExistentCwd) passes again; tests/unit/command_test.ts passes. Co-authored-by: Divy Srivastava <me@littledivy.com>
fibibot
left a comment
There was a problem hiding this comment.
The refactor in e0df4fb5 is exactly the right scoping. RunEnv.cwd is back to a real PathBuf with a separate set_cwd_on_command: bool companion, and allow_cwd_inherit is plumbed through compute_run_cmd_and_check_permissions → compute_run_env. Only op_node_spawn_child (the child_process.spawn Node-compat path) opts into true; op_spawn_child, op_spawn_sync, and the deprecated Deno.run go through with false and keep the existing strict "fail with FailedResolvingCwd" behavior. That preserves the nonExistentCwd unit test's contract while still satisfying test-cwd-enoent-repl.
test unit debug macos-aarch64 (the job that flagged the original regression) is now green (4m14s, https://github.com/denoland/deno/actions/runs/25012439805/job/73251949194). That's the precise validation the prior REQUEST_CHANGES needed.
Holding back from APPROVE on two grounds:
-
CI is still partially pending — the rest of
test unit debug(linux-aarch64/x86_64, macos-x86_64), most oftest node_compat debug, and the windows-aarch64 build are still running. Nothing's failed; just want to see the relevantext/processregression coverage land green before signing off. -
This PR adds a new top-level CLI flag (
--interactive/-iincli/args/flags.rs). Even hidden (hide(true)) it extends Deno's user-facing surface, and per fibibot's policy that's an escalation that wants a maintainer's deliberate sign-off rather than my autonomous approval. cc @bartlomieju — would appreciate your read on whether the-iflag is the right Node-compat shape or if you'd rather route it differently (e.g. only asnode -ivia thedeno nodeshim if/when that exists).
I'll re-confirm once CI fully lands green and you weigh in on the flag.
… arrive Enables tests/node_compat/runner/suite/test/parallel/test-cwd-enoent-repl.js Co-authored-by: Divy Srivastava <me@littledivy.com>
tools/lint.js refuses any new top-level entry; the harness lock file slipped into the sentinel commit (933c756) and tripped the lint debug check on every platform. The file is local-only state and is already gone from the working tree. Co-authored-by: Divy Srivastava <me@littledivy.com>
wpt release linux-x86_64 failed on the previous commit (55db3ed) on /websockets/stream/tentative/close.any.html?default — the timing-sensitive "incomplete closing handshake should be considered unclean close" subtest. WPT passed on e0df4fb (my refactor commit) and 55db3ed only deletes .claude/scheduled_tasks.lock, so the failure cannot be caused by code in this PR. Co-authored-by: Divy Srivastava <me@littledivy.com>
Summary
Enables
test-cwd-enoent-replin node_compat suite.Test plan
cargo test --test node_compat -- test-cwd-enoent-repl