path: fix inconsistent basename suffix stripping with trailing slashes by murataslan1 · Pull Request #62378 · nodejs/node

murataslan1 · 2026-03-21T23:51:58Z

Summary

path.basename() produced inconsistent results when stripping a suffix from paths with trailing slashes or leading path components. For example:

path.posix.basename('a', 'a')      // '' (correct)
path.posix.basename('a/', 'a')     // 'a' (bug — should be '')
path.posix.basename('/dd', 'dd')   // 'dd' (bug — should be '')
path.posix.basename('d/dd', 'dd')  // 'dd' (bug — should be '')

Root Cause

The suffix matching was done in a single pass with the path component scanning (introduced in #5123). This caused the suffix match to interact incorrectly with trailing slash handling and path separator detection.

Fix

Separate the two operations:

First resolve the basename (strip directory parts and trailing slashes)
Then strip the suffix from the resolved basename

This restores the pre-#5123 suffix stripping semantics while keeping the optimized path scanning. Both posix.basename() and win32.basename() are fixed.

Test Plan

Updated existing test expectations that encoded the buggy behavior
Added regression tests from the issue covering trailing slashes, leading path components, and suffix-with-separators cases

Refs: nodejs#53867 The run() function is exposed as a public API via node:test module. Previously, it accessed process.argv, process.execArgv, process.cwd(), and process.env directly, which meant programmatic API users could not fully control the test runner behavior. This change: - Captures process.argv, process.cwd(), process.env, and process.execArgv in the CLI entry point (main/test_runner.js) and passes them explicitly to run() as options - Adds a processExecArgv option for V8 flag propagation - Replaces process.env fallback in runTestFile() with opts.env - Replaces process.argv usage in getRunArgs() with globPatterns - Replaces process.env.NODE_TEST_CONTEXT check with env option - Maintains backwards compatibility: when options are not provided, run() falls back to process state as before

The basename() suffix matching was done in a single pass with the path component scanning, which caused incorrect results when: - The path had trailing slashes (e.g., 'a/') - The suffix contained path separators - The suffix equaled the basename after a path separator Fix by separating the two operations: first resolve the basename (stripping directory and trailing slashes), then strip the suffix from the result. This restores the pre-nodejs#5123 suffix stripping behavior while keeping the optimized path scanning. Fixes: nodejs#21358

nodejs-github-bot · 2026-03-21T23:52:04Z

Review requested:

@nodejs/path
@nodejs/test_runner

avivkeller · 2026-03-21T23:55:36Z

test/parallel/test-path-basename.js

-assert.strictEqual(path.basename('/aaa/bbb', 'bbb'), 'bbb');
-assert.strictEqual(path.basename('/aaa/bbb//', 'bbb'), 'bbb');
+assert.strictEqual(path.basename('/aaa/bbb', 'bbb'), '');
+assert.strictEqual(path.basename('/aaa/bbb//', 'bbb'), '');


Given that the previous behavior had tests, it had to have been intentional. If it's intentional, is there a solid rationale for changing the behavior, it's been like this for ages, changing it now seems like it would break thousands of libraries

In addition to that, I just checked with basename and the results I am getting are actually consistent with the existing (and not with the changes in this PR)

Fair point. The original issue reporter (@ChALkeR) also noted these inconsistencies were found via brute-force comparison, not from real-world breakage. And you're right that the existing tests encode this behavior intentionally.

Given the pushback and the risk of breaking existing libraries, I'm happy to close this PR. The current behavior has been stable for years and the edge cases (trailing slashes + suffix matching) are unlikely to affect real code.

If there's interest in pursuing this as a semver-major in the future, the approach here (separate basename resolution from suffix stripping) would be the way to do it.

Copilot

Pull request overview

This PR primarily restores consistent path.basename(path, suffix) behavior when the input path has trailing separators or leading path components, aligning suffix stripping semantics with the historical “strip after resolving basename” behavior. It also includes unrelated refactoring in the internal test runner to pass captured process state explicitly into run().

Changes:

Fix posix.basename() and win32.basename() to compute the basename first, then strip the suffix from that basename.
Update and expand test-path-basename expectations with regressions from #21358.
Refactor internal test runner entry/runner to pass globPatterns, cwd, env, and processExecArgv via options instead of reading process state directly.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
test/parallel/test-path-basename.js	Updates expected outputs and adds regression coverage for suffix stripping edge cases.
lib/path.js	Implements the basename-then-suffix-strip logic for both POSIX and Windows variants.
lib/internal/test_runner/runner.js	Introduces new options/defaulting for process state and adjusts isolated-process arg/env construction.
lib/internal/main/test_runner.js	Captures process state (argv/cwd/env/execArgv) and passes it into the runner.

Comments suppressed due to low confidence (1)

lib/internal/test_runner/runner.js:863

The recursion guard checks env.NODE_TEST_CONTEXT rather than the actual process environment. If a caller invokes run({ env: { ... } }) from within a test file (where process.env.NODE_TEST_CONTEXT is set) but omits that property in the provided env, this guard will no longer trigger and run() can execute recursively. Consider checking process.env.NODE_TEST_CONTEXT (or checking both) so the guard can’t be bypassed by passing a custom env object.

  if (isolation === 'process') {
    if (env.NODE_TEST_CONTEXT !== undefined) {
      process.emitWarning('node:test run() is being called recursively within a test file. skipping running files.');
      root.postRun();
      return root.reporter;
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-21T23:56:32Z

lib/internal/test_runner/runner.js

+  const userProvidedEnv = env !== undefined;
+  if (cwd === undefined) {
+    cwd = process.cwd();
+  }
+  if (env === undefined) {


run() now treats options.env: null as "user provided" and will throw in validateObject(env). Previously (given the earlier (opts.env || process.env) merge) null would effectively fall back to process.env. If null is meant to be accepted as "use default", update the userProvidedEnv logic/defaulting to treat null the same as undefined (or explicitly document this as a breaking change).

Suggested change

const userProvidedEnv = env !== undefined;

if (cwd === undefined) {

cwd = process.cwd();

}

if (env === undefined) {

const userProvidedEnv = env != null;

if (cwd === undefined) {

cwd = process.cwd();

}

if (env == null) {

Copilot · 2026-03-21T23:56:32Z

lib/internal/test_runner/runner.js

+  if (env === undefined) {
+    env = process.env;
+  }
+  if (processExecArgv === undefined) {


processExecArgv is introduced as an option and later assumed to be an array in getRunArgs() (used with ArrayPrototypeFilter). If a non-array value is provided, this will throw at runtime. Please add validation for options.processExecArgv (e.g., validateStringArray) when it is explicitly provided.

Suggested change

if (processExecArgv === undefined) {

if (processExecArgv !== undefined) {

validateStringArray(processExecArgv, 'options.processExecArgv');

} else {

Copilot · 2026-03-21T23:56:33Z

lib/internal/main/test_runner.js

+// Capture process state explicitly so run() does not need to access it.
 options.globPatterns = ArrayPrototypeSlice(process.argv, 1);
+options.cwd = process.cwd();
+options.env = process.env;
+options.processExecArgv = process.execArgv;


This file captures process.argv/cwd/env/execArgv into options for the test runner. These changes appear unrelated to the PR’s stated goal (fixing path.basename() suffix stripping) and aren’t mentioned in the PR description. If intentional, please update the PR description to include this behavioral refactor, or consider splitting it into a separate PR to keep the change focused.

murataslan1 · 2026-03-22T08:08:05Z

Closing this — the reviewers are right that changing this after so many years would be risky with little practical benefit. The edge cases here don't really come up in real code. Thanks for the feedback!

murataslan1 added 2 commits March 22, 2026 02:33

Copilot AI review requested due to automatic review settings March 21, 2026 23:51

murataslan1 mentioned this pull request Mar 21, 2026

Inconsistent behavior of path.basename(path, ext) #21358

Open

nodejs-github-bot added needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. test_runner Issues and PRs related to the test runner subsystem. labels Mar 21, 2026

Copilot started reviewing on behalf of murataslan1 March 21, 2026 23:52 View session

avivkeller added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 21, 2026

avivkeller requested changes Mar 21, 2026

View reviewed changes

Copilot AI reviewed Mar 21, 2026

View reviewed changes

murataslan1 closed this Mar 22, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

path: fix inconsistent basename suffix stripping with trailing slashes#62378

path: fix inconsistent basename suffix stripping with trailing slashes#62378
murataslan1 wants to merge 2 commits intonodejs:mainfrom
murataslan1:fix/path-basename-ext-inconsistency

murataslan1 commented Mar 21, 2026

Uh oh!

nodejs-github-bot commented Mar 21, 2026

Uh oh!

avivkeller Mar 21, 2026

Uh oh!

atlowChemi Mar 22, 2026

Uh oh!

murataslan1 Mar 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

murataslan1 commented Mar 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

-  if (processExecArgv === undefined) {
+  if (processExecArgv !== undefined) {
+    validateStringArray(processExecArgv, 'options.processExecArgv');
+  } else {

Uh oh!

Conversation

murataslan1 commented Mar 21, 2026

Summary

Root Cause

Fix

Test Plan

Uh oh!

nodejs-github-bot commented Mar 21, 2026

Uh oh!

avivkeller Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

atlowChemi Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

murataslan1 Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

murataslan1 commented Mar 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants