From cd536b25907e7e1ba42cc5a3684d11725fbda012 Mon Sep 17 00:00:00 2001 From: JATMN Date: Sat, 24 Oct 2026 09:35:51 -0700 Subject: [PATCH] test(skills): isolate staged install temp directory assertions (#2394) --- src/cli/handlers/skills.test.ts | 116 ++++++++++++++++++++++++++++++-- 1 file changed, 110 insertions(+), 6 deletions(-) diff --git a/src/cli/handlers/skills.test.ts b/src/cli/handlers/skills.test.ts index c7895633d..e41759319 100644 --- a/src/cli/handlers/skills.test.ts +++ b/src/cli/handlers/skills.test.ts @@ -13,6 +13,7 @@ import { import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'bun:test' +import { snapshotEnv } from '../../test/envSnapshot.js' import { isolateProcessExitCode } from '../../test/processExitCode.js' import { @@ -222,21 +223,33 @@ async function withMockedFetch( async function withTempDir(fn: (tempDir: string) => Promise): Promise { const tempDir = mkdtempSync(join(tmpdir(), 'openclaude-skill-install-test-')) + const tmpKeys = ['TMPDIR', 'TMP', 'TEMP'] as const + const restoreEnv = snapshotEnv([ + ...tmpKeys, 'OPENCLAUDE_SKILLS_REVOCATIONS_URL', + ]) + const nativeTmpEnv = tmpKeys.map(key => [key, Bun.env[key]] as const) + // Both the installer and the leak assertions must use this fixture's own + // directory, so other runs cannot change the staged-root snapshots. + // Bun's tmpdir reads its native environment even when another suite has + // replaced process.env, so update and restore both environment objects. + for (const key of tmpKeys) { + process.env[key] = tempDir + Bun.env[key] = tempDir + } // The revocation override is process-global input that the fixtures below // resolve against, and production gives it precedence over the sibling // revocations.json. Own it here: clear any value inherited from the host // process so each fixture reads its own list, and hand it back afterwards. - const inheritedRevocationsUrl = process.env.OPENCLAUDE_SKILLS_REVOCATIONS_URL delete process.env.OPENCLAUDE_SKILLS_REVOCATIONS_URL const restoreProcess = isolateProcessExitCode(0) try { return await fn(tempDir) } finally { try { - if (inheritedRevocationsUrl === undefined) { - delete process.env.OPENCLAUDE_SKILLS_REVOCATIONS_URL - } else { - process.env.OPENCLAUDE_SKILLS_REVOCATIONS_URL = inheritedRevocationsUrl + restoreEnv() + for (const [key, value] of nativeTmpEnv) { + if (value === undefined) delete Bun.env[key] + else Bun.env[key] = value } rmSync(tempDir, { recursive: true, force: true }) } finally { @@ -245,6 +258,61 @@ async function withTempDir(fn: (tempDir: string) => Promise): Promise { } } +test.serial('restores fixture environment and removes its temp root when a callback throws', async () => { + const keys = ['TMPDIR', 'TMP', 'TEMP', 'OPENCLAUDE_SKILLS_REVOCATIONS_URL'] + const inherited = keys.map(key => process.env[key]) + const inheritedNativeTmp = keys.slice(0, 3).map(key => Bun.env[key]) + const failure = new Error('fixture callback failed') + let fixtureRoot: string | undefined + await assert.rejects(withTempDir(async tempDir => { + fixtureRoot = tempDir + assert.equal(tmpdir(), tempDir) + throw failure + }), error => error === failure) + + assert.deepEqual(keys.map(key => process.env[key]), inherited) + assert.deepEqual(keys.slice(0, 3).map(key => Bun.env[key]), inheritedNativeTmp) + assert.ok(fixtureRoot) + assert.equal(existsSync(fixtureRoot), false) +}) + +for (const change of ['appears', 'disappears'] as const) { + test.serial(`ignores a foreign staged temp root that ${change} during install`, async () => { + const sharedTmpdir = tmpdir() + let foreignRoot: string | undefined + try { + if (change === 'disappears') { + foreignRoot = mkdtempSync(join(sharedTmpdir, 'openclaude-skill-install-test-')) + } + await withTempDir(async tempDir => { + const cwd = join(tempDir, 'project') + const source = writeSkillDir(join(tempDir, 'source')) + mkdirSync(cwd, { recursive: true }) + + const stagedBefore = stagedInstallTempDirs() + // Model another run creating a fixture, or finishing its cleanup, + // between this install's snapshots without relying on timing. + if (change === 'appears') { + foreignRoot = mkdtempSync(join(sharedTmpdir, 'openclaude-skill-install-test-')) + } else { + assert.ok(foreignRoot) + rmSync(foreignRoot, { recursive: true, force: true }) + } + await skillsInstallHandler(join(source, 'SKILL.md'), { projectDir: cwd }) + + assert.equal(process.exitCode, 0) + assert.equal( + readFileSync(join(cwd, '.openclaude', 'skills', 'sample-skill', 'SKILL.md'), 'utf8'), + VALID_SKILL, + ) + assertNoNewStagedInstallDirs(stagedBefore) + }) + } finally { + if (foreignRoot) rmSync(foreignRoot, { recursive: true, force: true }) + } + }) +} + test('formats skills list as an aligned human table', () => { const output = formatSkillsListForDisplay( [ @@ -1218,7 +1286,7 @@ test.serial('rejects direct HTTP URL installs without a sha256 pin', async () => }) }) -test.serial('cleans staged temp roots when markdown staging fails', async () => { +test.serial('rejects oversized skill names before staging', async () => { await withTempDir(async tempDir => { const cwd = join(tempDir, 'project') const source = join(tempDir, 'source', 'SKILL.md') @@ -1239,6 +1307,42 @@ test.serial('cleans staged temp roots when markdown staging fails', async () => }) }) +test.serial('cleans staged temp roots when markdown staging fails', async () => { + await acquireSharedMutationLock('skillsInstallHandler staging failure') + const originalFs = getFsImplementation() + try { + await withTempDir(async tempDir => { + const cwd = join(tempDir, 'project') + const source = writeSkillDir(join(tempDir, 'source')) + mkdirSync(cwd, { recursive: true }) + const stagedBefore = stagedInstallTempDirs() + let stagingWriteAttempted = false + setFsImplementation({ + ...originalFs, + writeFile: async () => { + // A valid skill must reach staging, then fail after mkdtemp. An + // invalid name is rejected before allocation and cannot test cleanup. + assert.equal(stagedInstallTempDirs().length, stagedBefore.length + 1) + stagingWriteAttempted = true + throw new Error('staging write failed') + }, + }) + try { + await skillsInstallHandler(join(source, 'SKILL.md'), { projectDir: cwd }) + + assert.ok(stagingWriteAttempted) + assert.equal(process.exitCode, 1) + assert.equal(existsSync(join(cwd, '.openclaude', 'skills')), false) + assertNoNewStagedInstallDirs(stagedBefore) + } finally { + setFsImplementation(originalFs) + } + }) + } finally { + releaseSharedMutationLock() + } +}) + test.serial('removes only the targeted project skill directory', async () => { await acquireSharedMutationLock('skillsRemoveHandler') const originalFs = getFsImplementation()