test(#3090): accept every canonical spelling of the temp root, not one platform's
Windows CI refused legitimate temp directories for the same reason macOS did, one commit earlier: cleanup() refused to remove a path outside os.tmpdir(): C:\Users\runneradmin\AppData\Local\Temp\bug-3491-7zisup GitHub's windows runners report os.tmpdir() in the 8.3 SHORT form (C:\Users\RUNNER~1\AppData\Local\Temp) while callers hold the expanded LONG form. fs.realpathSync() does not reliably expand 8.3 names; only fs.realpathSync.native() does. Casing can differ independently (C:\ vs c:\). Two platform-specific breakages from the same check is a sign the check was written against one spelling rather than the concept, so this stops patching symptoms. tmpRootCandidates() collects every variant derivable from os.tmpdir() — resolved, realpath'd, and native-realpath'd — each probe isolated in its own try/catch so an unavailable variant contributes nothing instead of crashing teardown, then deduped. A target is accepted under any of them, and the comparison folds case on win32 only, where casing genuinely varies. The error message still prints the original-case path. On this machine three variants collapse to two: /var/folders/.../T and /private/var/folders/.../T. Both spellings of a real temp dir are accepted and removed. The Linux matrix passed every one of these broken states — 30544 and then 30545, both lanes green — because /tmp has neither symlink indirection nor short names. The platform CI shards are the only thing that has caught any of it, which is worth stating plainly given what this branch is about. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -160,6 +160,21 @@ test('cleanup still removes a real os.tmpdir()-rooted directory (control)', () =
|
||||
assert.strictEqual(fs.existsSync(dir), false, 'os.tmpdir()-rooted directory should be removed');
|
||||
});
|
||||
|
||||
// ─── Test 6b: accepted-roots set always accepts a freshly-minted tmpdir ─────
|
||||
|
||||
test('cleanup accepts a freshly-created os.tmpdir()-rooted dir on any platform (accepted-roots invariant)', (t) => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-roots-'));
|
||||
t.after(() => {
|
||||
if (fs.existsSync(dir)) cleanup(dir);
|
||||
});
|
||||
|
||||
assert.doesNotThrow(
|
||||
() => cleanup(dir),
|
||||
'cleanup must accept a directory created directly under os.tmpdir(), regardless of platform-specific canonical spelling (8.3 short/long names, /var symlink, drive-letter case)'
|
||||
);
|
||||
assert.strictEqual(fs.existsSync(dir), false, 'temp dir should be removed, not refused');
|
||||
});
|
||||
|
||||
// ─── Test 6: realpath'd os.tmpdir() form is not refused (regression) ────────
|
||||
|
||||
test('cleanup accepts a realpath()d temp dir even when it differs from the raw path (macOS /var -> /private/var)', (t) => {
|
||||
|
||||
@@ -206,6 +206,40 @@ function createTempGitProject(prefix = 'gsd-test-') {
|
||||
return createFixture({ prefix, planning: true, git: true, projectDoc: true });
|
||||
}
|
||||
|
||||
// The OS temp root has several canonical spellings, and a path a caller
|
||||
// legitimately passes to cleanup() may arrive in any of them: macOS resolves
|
||||
// os.tmpdir() under /var/folders/... but /var is a symlink to /private/var;
|
||||
// Windows CI runners report os.tmpdir() in the 8.3 SHORT form
|
||||
// (C:\Users\RUNNER~1\AppData\Local\Temp) while the caller's path is the
|
||||
// expanded LONG form, and fs.realpathSync() does not reliably expand 8.3
|
||||
// short names there — only fs.realpathSync.native() does; drive-letter and
|
||||
// path casing can also differ (C:\ vs c:\). Collect every variant we can
|
||||
// derive from os.tmpdir() and accept a target under any of them. Each probe
|
||||
// is wrapped in its own try/catch — none of them may throw and crash
|
||||
// cleanup(), they just contribute nothing if unavailable.
|
||||
function tmpRootCandidates() {
|
||||
const roots = [];
|
||||
try {
|
||||
roots.push(path.resolve(os.tmpdir()));
|
||||
} catch (_) { /* os.tmpdir() unavailable — skip this variant */ }
|
||||
try {
|
||||
roots.push(fs.realpathSync(os.tmpdir()));
|
||||
} catch (_) { /* temp root unreadable — skip this variant */ }
|
||||
try {
|
||||
roots.push(fs.realpathSync.native(os.tmpdir()));
|
||||
} catch (_) { /* native realpath unavailable/unreadable — skip this variant */ }
|
||||
const isWindows = process.platform === 'win32';
|
||||
const seen = new Set();
|
||||
const deduped = [];
|
||||
for (const root of roots) {
|
||||
const key = isWindows ? root.toLowerCase() : root;
|
||||
if (seen.has(key)) continue;
|
||||
seen.add(key);
|
||||
deduped.push(root);
|
||||
}
|
||||
return deduped;
|
||||
}
|
||||
|
||||
function cleanup(tmpDir) {
|
||||
if (typeof tmpDir !== 'string' || tmpDir.length === 0) return;
|
||||
const target = path.resolve(tmpDir);
|
||||
@@ -216,26 +250,21 @@ function cleanup(tmpDir) {
|
||||
// its own tree and get force-deleted. Hoisted above both the chdir and the
|
||||
// rmSync so an out-of-tmpdir path is refused before either can run.
|
||||
//
|
||||
// Two acceptable roots, not one: on macOS os.tmpdir() returns a path under
|
||||
// /var/folders/... but /var is a symlink to /private/var, and path.resolve()
|
||||
// does not resolve symlinks. A caller that passed the REALPATH'd form of a
|
||||
// temp dir (e.g. via fs.realpathSync(), or via process.cwd() after chdir-ing
|
||||
// into a realpath'd dir) would resolve to /private/var/folders/... and get
|
||||
// wrongly refused by a check against only path.resolve(os.tmpdir()). Build
|
||||
// the accepted-roots set from both the raw and realpath'd forms of
|
||||
// os.tmpdir() — realpathSync is wrapped in try/catch because it throws if
|
||||
// the temp root is momentarily missing, and this guard must never crash
|
||||
// cleanup() over that. Do NOT realpath `target` itself: cleanup() is
|
||||
// legitimately called on already-deleted or never-created dirs, and
|
||||
// realpathSync throws ENOENT on a missing path.
|
||||
const tmpRoots = [path.resolve(os.tmpdir())];
|
||||
try {
|
||||
const realTmpRoot = fs.realpathSync(os.tmpdir());
|
||||
if (!tmpRoots.includes(realTmpRoot)) tmpRoots.push(realTmpRoot);
|
||||
} catch (_) { /* temp root unreadable — fall back to the resolved form only */ }
|
||||
const isTmpPath = tmpRoots.some(
|
||||
(root) => target === root || target.startsWith(`${root}${path.sep}`)
|
||||
);
|
||||
// Do NOT realpath `target` itself: cleanup() is legitimately called on
|
||||
// already-deleted or never-created dirs, and realpathSync throws ENOENT on
|
||||
// a missing path. Comparison is case-insensitive on Windows (drive-letter
|
||||
// and path casing vary there) and case-sensitive everywhere else; the
|
||||
// error message below always prints the original-case target.
|
||||
const isWindows = process.platform === 'win32';
|
||||
const tmpRoots = tmpRootCandidates();
|
||||
const targetForCompare = isWindows ? target.toLowerCase() : target;
|
||||
const isTmpPath = tmpRoots.some((root) => {
|
||||
const rootForCompare = isWindows ? root.toLowerCase() : root;
|
||||
return (
|
||||
targetForCompare === rootForCompare ||
|
||||
targetForCompare.startsWith(`${rootForCompare}${path.sep}`)
|
||||
);
|
||||
});
|
||||
if (!isTmpPath) {
|
||||
throw new Error(`cleanup() refused to remove a path outside os.tmpdir(): ${target}`);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user