From ca0b5f23eee768a930aa671fbf2c6babbdf25a77 Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Tue, 3 Feb 2026 22:45:02 +0000 Subject: [PATCH] address review feedback - expandTilde: always duplicate input to ensure memory ownership - expandTilde: fix Windows path separator logic - tests: use cloned env to avoid race conditions with shared object - tests: use 'await using' pattern for proper resource cleanup Co-Authored-By: Claude Opus 4.5 --- src/string/immutable.zig | 16 ++- test/regression/issue/26715.test.ts | 193 +++++++++++++--------------- 2 files changed, 97 insertions(+), 112 deletions(-) diff --git a/src/string/immutable.zig b/src/string/immutable.zig index 212d73a15d..bd3b477d86 100644 --- a/src/string/immutable.zig +++ b/src/string/immutable.zig @@ -2385,18 +2385,22 @@ pub const log = bun.Output.scoped(.STR, .hidden); pub const grapheme = @import("./immutable/grapheme.zig"); /// Expands a leading tilde (~) in a path to the user's home directory. -/// Returns the original string if it doesn't start with ~ or if the home directory cannot be determined. +/// Always returns a newly allocated string owned by the allocator. /// The tilde must be followed by a path separator or be the entire string (e.g., "~" or "~/foo"). +/// Paths like "~username" are not expanded but are still duplicated. pub fn expandTilde(allocator: std.mem.Allocator, path: []const u8) OOM![]const u8 { if (path.len == 0 or path[0] != '~') { - return path; + return try allocator.dupe(u8, path); } - // Must be just "~" or "~/..." (not "~foo" which means another user's home) - if (path.len > 1 and path[1] != '/' and (Environment.isWindows and path[1] != '\\') == false) { - return path; + // Must be just "~" or "~/..." or "~\..." on Windows (not "~foo" which means another user's home) + if (path.len > 1) { + const is_separator = path[1] == '/' or (Environment.isWindows and path[1] == '\\'); + if (!is_separator) { + return try allocator.dupe(u8, path); + } } - const home_dir = bun.env_var.HOME.get() orelse return path; + const home_dir = bun.env_var.HOME.get() orelse return try allocator.dupe(u8, path); if (path.len == 1) { // Just "~" diff --git a/test/regression/issue/26715.test.ts b/test/regression/issue/26715.test.ts index e819698e04..a270902d34 100644 --- a/test/regression/issue/26715.test.ts +++ b/test/regression/issue/26715.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { rm } from "fs/promises"; -import { bunExe, bunEnv as env, stderrForInstall, tempDirWithFiles } from "harness"; +import { bunEnv, bunExe, stderrForInstall, tempDirWithFiles } from "harness"; import { join } from "path"; // https://github.com/oven-sh/bun/issues/26715 @@ -16,34 +16,31 @@ describe("tilde expansion in cache paths", () => { // Remove any bunfig.toml that might override the .npmrc setting await rm(join(testDir, "bunfig.toml"), { force: true }); - const originalCacheDir = env.BUN_INSTALL_CACHE_DIR; - delete env.BUN_INSTALL_CACHE_DIR; + // Clone env to avoid mutating shared object + const testEnv = { ...bunEnv }; + delete testEnv.BUN_INSTALL_CACHE_DIR; - try { - const { stdout, stderr, exited } = Bun.spawn({ - cmd: [bunExe(), "pm", "cache"], - cwd: testDir, - env, - stdout: "pipe", - stderr: "pipe", - }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "pm", "cache"], + cwd: testDir, + env: testEnv, + stdout: "pipe", + stderr: "pipe", + }); - const out = (await stdout.text()).trim(); - const err = stderrForInstall(await stderr.text()); + const out = (await proc.stdout.text()).trim(); + const err = stderrForInstall(await proc.stderr.text()); - expect(err).toBeEmpty(); - // Should NOT contain literal "~" - it should be expanded to the home directory - expect(out).not.toContain("/~/"); - expect(out).not.toStartWith("~"); - // Should contain the home directory path - const homeDir = process.env.HOME || process.env.USERPROFILE; - expect(out).toStartWith(homeDir!); - expect(out).toEndWith(".bun-test-cache-dir"); + expect(err).toBeEmpty(); + // Should NOT contain literal "~" - it should be expanded to the home directory + expect(out).not.toContain("/~/"); + expect(out).not.toStartWith("~"); + // Should contain the home directory path + const homeDir = process.env.HOME || process.env.USERPROFILE; + expect(out).toStartWith(homeDir!); + expect(out).toEndWith(".bun-test-cache-dir"); - expect(await exited).toBe(0); - } finally { - env.BUN_INSTALL_CACHE_DIR = originalCacheDir; - } + expect(await proc.exited).toBe(0); }); test("bunfig.toml cache string expands tilde to home directory", async () => { @@ -52,32 +49,28 @@ describe("tilde expansion in cache paths", () => { "package.json": JSON.stringify({ name: "foo", version: "1.0.0" }), }); - const originalCacheDir = env.BUN_INSTALL_CACHE_DIR; - delete env.BUN_INSTALL_CACHE_DIR; + const testEnv = { ...bunEnv }; + delete testEnv.BUN_INSTALL_CACHE_DIR; - try { - const { stdout, stderr, exited } = Bun.spawn({ - cmd: [bunExe(), "pm", "cache"], - cwd: testDir, - env, - stdout: "pipe", - stderr: "pipe", - }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "pm", "cache"], + cwd: testDir, + env: testEnv, + stdout: "pipe", + stderr: "pipe", + }); - const out = (await stdout.text()).trim(); - const err = stderrForInstall(await stderr.text()); + const out = (await proc.stdout.text()).trim(); + const err = stderrForInstall(await proc.stderr.text()); - expect(err).toBeEmpty(); - expect(out).not.toContain("/~/"); - expect(out).not.toStartWith("~"); - const homeDir = process.env.HOME || process.env.USERPROFILE; - expect(out).toStartWith(homeDir!); - expect(out).toEndWith(".bun-test-cache-dir"); + expect(err).toBeEmpty(); + expect(out).not.toContain("/~/"); + expect(out).not.toStartWith("~"); + const homeDir = process.env.HOME || process.env.USERPROFILE; + expect(out).toStartWith(homeDir!); + expect(out).toEndWith(".bun-test-cache-dir"); - expect(await exited).toBe(0); - } finally { - env.BUN_INSTALL_CACHE_DIR = originalCacheDir; - } + expect(await proc.exited).toBe(0); }); test("bunfig.toml cache.dir expands tilde to home directory", async () => { @@ -86,32 +79,28 @@ describe("tilde expansion in cache paths", () => { "package.json": JSON.stringify({ name: "foo", version: "1.0.0" }), }); - const originalCacheDir = env.BUN_INSTALL_CACHE_DIR; - delete env.BUN_INSTALL_CACHE_DIR; + const testEnv = { ...bunEnv }; + delete testEnv.BUN_INSTALL_CACHE_DIR; - try { - const { stdout, stderr, exited } = Bun.spawn({ - cmd: [bunExe(), "pm", "cache"], - cwd: testDir, - env, - stdout: "pipe", - stderr: "pipe", - }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "pm", "cache"], + cwd: testDir, + env: testEnv, + stdout: "pipe", + stderr: "pipe", + }); - const out = (await stdout.text()).trim(); - const err = stderrForInstall(await stderr.text()); + const out = (await proc.stdout.text()).trim(); + const err = stderrForInstall(await proc.stderr.text()); - expect(err).toBeEmpty(); - expect(out).not.toContain("/~/"); - expect(out).not.toStartWith("~"); - const homeDir = process.env.HOME || process.env.USERPROFILE; - expect(out).toStartWith(homeDir!); - expect(out).toEndWith(".bun-test-cache-dir"); + expect(err).toBeEmpty(); + expect(out).not.toContain("/~/"); + expect(out).not.toStartWith("~"); + const homeDir = process.env.HOME || process.env.USERPROFILE; + expect(out).toStartWith(homeDir!); + expect(out).toEndWith(".bun-test-cache-dir"); - expect(await exited).toBe(0); - } finally { - env.BUN_INSTALL_CACHE_DIR = originalCacheDir; - } + expect(await proc.exited).toBe(0); }); test("paths without tilde are not affected", async () => { @@ -120,28 +109,24 @@ describe("tilde expansion in cache paths", () => { "package.json": JSON.stringify({ name: "foo", version: "1.0.0" }), }); - const originalCacheDir = env.BUN_INSTALL_CACHE_DIR; - delete env.BUN_INSTALL_CACHE_DIR; + const testEnv = { ...bunEnv }; + delete testEnv.BUN_INSTALL_CACHE_DIR; - try { - const { stdout, stderr, exited } = Bun.spawn({ - cmd: [bunExe(), "pm", "cache"], - cwd: testDir, - env, - stdout: "pipe", - stderr: "pipe", - }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "pm", "cache"], + cwd: testDir, + env: testEnv, + stdout: "pipe", + stderr: "pipe", + }); - const out = (await stdout.text()).trim(); - const err = stderrForInstall(await stderr.text()); + const out = (await proc.stdout.text()).trim(); + const err = stderrForInstall(await proc.stderr.text()); - expect(err).toBeEmpty(); - expect(out).toBe("/tmp/absolute-cache-dir"); + expect(err).toBeEmpty(); + expect(out).toBe("/tmp/absolute-cache-dir"); - expect(await exited).toBe(0); - } finally { - env.BUN_INSTALL_CACHE_DIR = originalCacheDir; - } + expect(await proc.exited).toBe(0); }); test("~username paths are not expanded (only ~ is expanded)", async () => { @@ -152,28 +137,24 @@ describe("tilde expansion in cache paths", () => { "package.json": JSON.stringify({ name: "foo", version: "1.0.0" }), }); - const originalCacheDir = env.BUN_INSTALL_CACHE_DIR; - delete env.BUN_INSTALL_CACHE_DIR; + const testEnv = { ...bunEnv }; + delete testEnv.BUN_INSTALL_CACHE_DIR; - try { - const { stdout, stderr, exited } = Bun.spawn({ - cmd: [bunExe(), "pm", "cache"], - cwd: testDir, - env, - stdout: "pipe", - stderr: "pipe", - }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "pm", "cache"], + cwd: testDir, + env: testEnv, + stdout: "pipe", + stderr: "pipe", + }); - const out = (await stdout.text()).trim(); - const err = stderrForInstall(await stderr.text()); + const out = (await proc.stdout.text()).trim(); + const err = stderrForInstall(await proc.stderr.text()); - expect(err).toBeEmpty(); - // Should still contain ~otheruser since we don't expand that form - expect(out).toContain("~otheruser"); + expect(err).toBeEmpty(); + // Should still contain ~otheruser since we don't expand that form + expect(out).toContain("~otheruser"); - expect(await exited).toBe(0); - } finally { - env.BUN_INSTALL_CACHE_DIR = originalCacheDir; - } + expect(await proc.exited).toBe(0); }); });