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 <noreply@anthropic.com>
This commit is contained in:
Claude Bot
2026-02-03 22:45:02 +00:00
parent 1a0785d486
commit ca0b5f23ee
2 changed files with 97 additions and 112 deletions

View File

@@ -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 "~"

View File

@@ -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);
});
});