From 5a1fbe03e62f354a972871336cbf541f27cf4dd9 Mon Sep 17 00:00:00 2001 From: JATMN Date: Wed, 21 Oct 2026 07:05:35 -0700 Subject: [PATCH] fix(tests): isolate skills homes and stabilize reload assertions (#2307) Co-authored-by: OpenClaude CI Maintenance --- src/skills/loadSkillsDir.test.ts | 57 ++++++++++++-- src/utils/skills/skillChangeDetector.test.ts | 82 +++++++++++++++----- 2 files changed, 112 insertions(+), 27 deletions(-) diff --git a/src/skills/loadSkillsDir.test.ts b/src/skills/loadSkillsDir.test.ts index 34b9c4d7f..c9bdb41c6 100644 --- a/src/skills/loadSkillsDir.test.ts +++ b/src/skills/loadSkillsDir.test.ts @@ -5,6 +5,7 @@ import { mkdirSync, readdirSync, readFileSync, + realpathSync, rmSync, statSync, symlinkSync, @@ -131,17 +132,32 @@ function setRealFilesystemForTest(): ReturnType { return originalFs } -function setConfigDirEnv(configDir: string): void { +function setConfigDirEnv(configDir: string, homeDir = join(configDir, 'home')): void { + mkdirSync(homeDir, { recursive: true }) + process.env.HOME = homeDir + process.env.USERPROFILE = homeDir + // Bun caches os.homedir(), so changing HOME alone does not isolate siblings. + setSiblingSkillsHomeDirForTesting(homeDir) setClaudeConfigHomeDirForTesting(undefined) process.env.OPENCLAUDE_CONFIG_DIR = configDir delete process.env.CLAUDE_CONFIG_DIR } function restoreConfigDirEnv(original: { + home: string | undefined + userProfile: string | undefined openClaudeConfigDir: string | undefined claudeConfigDir: string | undefined configHomeOverride: string | undefined }): void { + for (const [key, value] of Object.entries({ + HOME: original.home, + USERPROFILE: original.userProfile, + })) { + if (value === undefined) delete process.env[key] + else process.env[key] = value + } + setSiblingSkillsHomeDirForTesting(undefined) setClaudeConfigHomeDirForTesting(original.configHomeOverride) if (original.openClaudeConfigDir === undefined) { @@ -162,6 +178,8 @@ test.serial('loads flat and nested skills with colon namespaces', async () => { const configDir = mkdtempSync(join(tmpdir(), 'openclaude-skills-')) const cwd = join(configDir, 'workspace') const originalConfigDir = { + home: process.env.HOME, + userProfile: process.env.USERPROFILE, openClaudeConfigDir: process.env.OPENCLAUDE_CONFIG_DIR, claudeConfigDir: process.env.CLAUDE_CONFIG_DIR, configHomeOverride: getClaudeConfigHomeDirOverrideForTesting(), @@ -179,6 +197,10 @@ test.serial('loads flat and nested skills with colon namespaces', async () => { clearSkillAndConfigCaches() const skills = await getSkillDirCommands(cwd) + assert.equal( + skills.some(skill => skill.type === 'prompt' && skill.siblingSkillsHome), + false, + ) const fixtureSkillsRoot = join(configDir, '.openclaude', 'skills') const promptSkills = skills.filter( ( @@ -227,6 +249,8 @@ test.serial('ignores legacy .claude project skills when .openclaude skills exist const configDir = mkdtempSync(join(tmpdir(), 'openclaude-skills-')) const cwd = join(configDir, 'workspace') const originalConfigDir = { + home: process.env.HOME, + userProfile: process.env.USERPROFILE, openClaudeConfigDir: process.env.OPENCLAUDE_CONFIG_DIR, claudeConfigDir: process.env.CLAUDE_CONFIG_DIR, configHomeOverride: getClaudeConfigHomeDirOverrideForTesting(), @@ -275,6 +299,8 @@ test.serial('loads persisted registry trust metadata from skill.json', async () const configDir = mkdtempSync(join(tmpdir(), 'openclaude-skills-')) const cwd = join(configDir, 'workspace') const originalConfigDir = { + home: process.env.HOME, + userProfile: process.env.USERPROFILE, openClaudeConfigDir: process.env.OPENCLAUDE_CONFIG_DIR, claudeConfigDir: process.env.CLAUDE_CONFIG_DIR, configHomeOverride: getClaudeConfigHomeDirOverrideForTesting(), @@ -323,6 +349,8 @@ test.serial('project skills are ordered before user skills with the same name', const configDir = mkdtempSync(join(tmpdir(), 'openclaude-skills-')) const cwd = join(configDir, 'workspace') const originalConfigDir = { + home: process.env.HOME, + userProfile: process.env.USERPROFILE, openClaudeConfigDir: process.env.OPENCLAUDE_CONFIG_DIR, claudeConfigDir: process.env.CLAUDE_CONFIG_DIR, configHomeOverride: getClaudeConfigHomeDirOverrideForTesting(), @@ -459,28 +487,31 @@ async function withSiblingHomeFixture( const rootDir = mkdtempSync(join(tmpdir(), 'openclaude-sibling-skills-')) const homeDir = join(rootDir, 'home') const configDir = join(homeDir, '.openclaude') - // Inside the fake home so project-dir traversal stops at it. + // Keep project and user skills in the same disposable fixture. const cwd = join(homeDir, 'workspace') const originalConfigDir = { + home: process.env.HOME, + userProfile: process.env.USERPROFILE, openClaudeConfigDir: process.env.OPENCLAUDE_CONFIG_DIR, claudeConfigDir: process.env.CLAUDE_CONFIG_DIR, configHomeOverride: getClaudeConfigHomeDirOverrideForTesting(), } const originalOriginalCwd = getOriginalCwd() + const originalCwd = process.cwd() const originalSettingsState = enableUserAndProjectSettingSources() const originalFs = setRealFilesystemForTest() try { mkdirSync(cwd, { recursive: true }) mkdirSync(configDir, { recursive: true }) - setSiblingSkillsHomeDirForTesting(homeDir) - setConfigDirEnv(configDir) + setConfigDirEnv(configDir, homeDir) + process.chdir(cwd) setOriginalCwd(cwd) clearSkillAndConfigCaches() await run({ homeDir, configDir, cwd }) } finally { try { - setSiblingSkillsHomeDirForTesting(undefined) + process.chdir(originalCwd) restoreConfigDirEnv(originalConfigDir) setOriginalCwd(originalOriginalCwd) setFsImplementation(originalFs) @@ -1103,6 +1134,7 @@ for (const home of ['.agents', '.claude'] as const) { const plainFile = join(writeSiblingSkill(homeDir, home, 'plain'), 'SKILL.md') symlinkSync(target, join(homeDir, home, 'skills', 'linked'), process.platform === 'win32' ? 'junction' : 'dir') const ready: Promise[] = [] + const changedPaths = new Set() await detector.resetForTesting({ stabilityThreshold: 10, pollInterval: 10, chokidarInterval: 10, reloadDebounce: 10, reloadCooldown: 10, @@ -1111,6 +1143,7 @@ for (const home of ['.agents', '.claude'] as const) { executeConfigChangeHooks: async () => [], watch: (paths, options) => { const watcher = chokidar.watch(paths, options) + watcher.on('change', path => changedPaths.add(realpathSync(path))) ready.push(new Promise(resolve => watcher.once('ready', resolve))) return watcher }, @@ -1127,12 +1160,20 @@ for (const home of ['.agents', '.claude'] as const) { const before = notifications const deadline = Date.now() + 2500 let edit = 0 - while (Date.now() < deadline) { + const watchedFile = realpathSync(file) + changedPaths.delete(watchedFile) + // Retry only until Chokidar observes the write. Retrying until reload + // can enqueue another edit or continually restart the reload debounce. + while (!changedPaths.has(watchedFile) && Date.now() < deadline) { writeFileSync(file, `---\ndescription: ${marker}\n---\nInstructions ${marker} ${edit++}\n`) await Bun.sleep(50) - if (notifications > before && (await invokedText(name)).includes(marker)) return } - assert.fail(`No refreshed invocation for ${name}: ${marker}`) + assert.ok(changedPaths.has(watchedFile), `No file event for ${name}: ${marker}`) + while (notifications === before && Date.now() < deadline) { + await Bun.sleep(10) + } + assert.ok(notifications > before, `No reload for ${name}: ${marker}`) + assert.ok((await invokedText(name)).includes(marker)) } try { await detector.initialize() diff --git a/src/utils/skills/skillChangeDetector.test.ts b/src/utils/skills/skillChangeDetector.test.ts index fa0236f7f..cb3ffa234 100644 --- a/src/utils/skills/skillChangeDetector.test.ts +++ b/src/utils/skills/skillChangeDetector.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test' +import { afterEach, beforeEach, describe, expect, jest, mock, test } from 'bun:test' import * as platformPath from 'path' import chokidar, { type FSWatcher } from 'chokidar' import { type Stats, mkdirSync, mkdtempSync, rmSync, statSync, writeFileSync } from 'fs' @@ -33,6 +33,9 @@ let chokidarWatch = mock(() => ({ close: mock(async () => {}), })) let activeDetector: SkillChangeDetectorModule | null = null +let testHome: string +let originalCwd: string +let originalEnv: Record function installMocks(): void { clearCommandsCache = mock(() => {}) @@ -91,8 +94,29 @@ function sleep(ms: number): Promise { return new Promise(resolve => setTimeout(resolve, ms)) } +async function advanceTime(ms: number): Promise { + await new Promise(resolve => setImmediate(resolve)) + jest.advanceTimersByTime(ms) + // Timer callbacks await hooks; let their promise continuations finish before + // assertions or the next advance (which may depend on the new cooldown). + await new Promise(resolve => setImmediate(resolve)) +} + beforeEach(async () => { await acquireSharedMutationLock('utils/skills/skillChangeDetector.test.ts') + originalCwd = process.cwd() + originalEnv = { + HOME: process.env.HOME, + USERPROFILE: process.env.USERPROFILE, + OPENCLAUDE_CONFIG_DIR: process.env.OPENCLAUDE_CONFIG_DIR, + CLAUDE_CONFIG_DIR: process.env.CLAUDE_CONFIG_DIR, + } + testHome = mkdtempSync(platformPath.join(tmpdir(), 'skill-detector-home-')) + process.env.HOME = testHome + process.env.USERPROFILE = testHome + process.env.OPENCLAUDE_CONFIG_DIR = platformPath.join(testHome, '.openclaude') + delete process.env.CLAUDE_CONFIG_DIR + process.chdir(testHome) installMocks() }) @@ -103,6 +127,13 @@ afterEach(async () => { activeDetector = null mock.restore() } finally { + jest.useRealTimers() + process.chdir(originalCwd) + for (const [key, value] of Object.entries(originalEnv)) { + if (value === undefined) delete process.env[key] + else process.env[key] = value + } + rmSync(testHome, { recursive: true, force: true }) releaseSharedMutationLock() } }) @@ -363,6 +394,7 @@ describe('skillChangeDetector reload batching', () => { const detector = await importFreshModule() let realWatcher: FSWatcher | undefined const hookPaths: string[] = [] + const changedPaths = new Set() detector._setDependenciesForTesting({ getSkillsPath: () => own, getAdditionalDirectoriesForClaudeMd: () => [], @@ -376,6 +408,7 @@ describe('skillChangeDetector reload batching', () => { resetSentSkillNames, watch: (paths: string[], options: Parameters[1]) => { realWatcher = chokidar.watch(paths, options) + realWatcher.on('change', path => changedPaths.add(path)) return realWatcher }, }) @@ -400,16 +433,20 @@ describe('skillChangeDetector reload batching', () => { } const editUntilHook = async (path: string, content: string) => { // Chokidar's ready marks traversal completion, but Bun may still be - // initializing its polling baseline. Retry edits until one is observed. - // A retry already in flight when the first hook lands can still produce - // one more, so assert that the edit was seen rather than an exact count. + // initializing its polling baseline. Stop retrying when the file event + // arrives, then wait for its hook without resetting the reload debounce. const start = hookPaths.length const deadline = Date.now() + 2000 let edit = 0 - while (!hookPaths.slice(start).includes(path) && Date.now() < deadline) { + changedPaths.delete(path) + while (!changedPaths.has(path) && Date.now() < deadline) { writeFileSync(path, `${content} ${edit++}`) await sleep(50) } + expect(changedPaths.has(path)).toBe(true) + while (!hookPaths.slice(start).includes(path) && Date.now() < deadline) { + await sleep(10) + } // This test checks source eligibility; batching has dedicated tests below. expect(hookPaths.slice(start)).toContain(path) await waitForQuietHooks() @@ -440,6 +477,7 @@ describe('skillChangeDetector reload batching', () => { test('batches rapid reload requests into one hook/cache clear/notification', async () => { const detector = await importFreshModule() + jest.useFakeTimers() await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 20 }) let notifications = 0 @@ -451,7 +489,7 @@ describe('skillChangeDetector reload batching', () => { detector._scheduleReloadForTesting('/tmp/skills/b/SKILL.md') detector._scheduleReloadForTesting('/tmp/skills/c/SKILL.md') - await sleep(30) + await advanceTime(30) expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) expect(clearCommandsCache).toHaveBeenCalledTimes(1) @@ -465,6 +503,7 @@ describe('skillChangeDetector reload batching', () => { test('delays a second burst until the reload cooldown has elapsed', async () => { const detector = await importFreshModule() + jest.useFakeTimers() await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 60 }) let notifications = 0 @@ -473,14 +512,14 @@ describe('skillChangeDetector reload batching', () => { }) detector._scheduleReloadForTesting('/tmp/skills/first/SKILL.md') - await sleep(20) + await advanceTime(20) expect(notifications).toBe(1) detector._scheduleReloadForTesting('/tmp/skills/second/SKILL.md') - await sleep(20) + await advanceTime(20) expect(notifications).toBe(1) - await sleep(60) + await advanceTime(60) expect(notifications).toBe(2) expect(executeConfigChangeHooks).toHaveBeenCalledTimes(2) @@ -490,6 +529,7 @@ describe('skillChangeDetector reload batching', () => { test('does not start cooldown when a ConfigChange hook blocks reload', async () => { const detector = await importFreshModule() + jest.useFakeTimers() await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 500 }) let notifications = 0 @@ -499,13 +539,13 @@ describe('skillChangeDetector reload batching', () => { hookResults = [{ blocked: true }] detector._scheduleReloadForTesting('/tmp/skills/blocked/SKILL.md') - await sleep(20) + await advanceTime(20) expect(notifications).toBe(0) expect(clearCommandsCache).not.toHaveBeenCalled() hookResults = [] detector._scheduleReloadForTesting('/tmp/skills/allowed/SKILL.md') - await sleep(20) + await advanceTime(20) expect(notifications).toBe(1) expect(clearCommandsCache).toHaveBeenCalledTimes(1) @@ -516,6 +556,7 @@ describe('skillChangeDetector reload batching', () => { test('queues events during an in-flight reload before cache clear and notification', async () => { const detector = await importFreshModule() + jest.useFakeTimers() await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 60 }) let releaseFirstHook: (() => void) | undefined @@ -541,21 +582,21 @@ describe('skillChangeDetector reload batching', () => { }) detector._scheduleReloadForTesting('/tmp/skills/first/SKILL.md') - await sleep(20) + await advanceTime(20) expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) detector._scheduleReloadForTesting('/tmp/skills/second/SKILL.md') - await sleep(30) + await advanceTime(30) expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) releaseFirstHook?.() - await sleep(20) + await advanceTime(20) expect(notifications).toBe(0) expect(clearCommandsCache).not.toHaveBeenCalled() expect(executeConfigChangeHooks).toHaveBeenCalledTimes(2) releaseSecondHook?.() - await sleep(30) + await advanceTime(30) expect(notifications).toBe(1) expect(executeConfigChangeHooks).toHaveBeenCalledTimes(2) @@ -565,6 +606,7 @@ describe('skillChangeDetector reload batching', () => { test('dispose prevents an in-flight reload from clearing caches after hook resolution', async () => { const detector = await importFreshModule() + jest.useFakeTimers() await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 20 }) let releaseHook: (() => void) | undefined @@ -581,12 +623,12 @@ describe('skillChangeDetector reload batching', () => { }) detector._scheduleReloadForTesting('/tmp/skills/in-flight/SKILL.md') - await sleep(20) + await advanceTime(20) expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) await detector.dispose() releaseHook?.() - await sleep(20) + await advanceTime(20) expect(clearCommandsCache).not.toHaveBeenCalled() expect(resetSentSkillNames).not.toHaveBeenCalled() @@ -595,6 +637,7 @@ describe('skillChangeDetector reload batching', () => { test('dispose prevents later reload scheduling from running hooks', async () => { const detector = await importFreshModule() + jest.useFakeTimers() await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 20 }) let notifications = 0 @@ -604,7 +647,7 @@ describe('skillChangeDetector reload batching', () => { await detector.dispose() detector._scheduleReloadForTesting('/tmp/skills/after-dispose/SKILL.md') - await sleep(20) + await advanceTime(20) expect(executeConfigChangeHooks).not.toHaveBeenCalled() expect(clearCommandsCache).not.toHaveBeenCalled() @@ -613,11 +656,12 @@ describe('skillChangeDetector reload batching', () => { test('resetForTesting clears pending reload timers', async () => { const detector = await importFreshModule() + jest.useFakeTimers() await detector.resetForTesting({ reloadDebounce: 20, reloadCooldown: 20 }) detector._scheduleReloadForTesting('/tmp/skills/pending/SKILL.md') await detector.resetForTesting({ reloadDebounce: 5, reloadCooldown: 5 }) - await sleep(30) + await advanceTime(30) expect(executeConfigChangeHooks).not.toHaveBeenCalled() expect(clearCommandsCache).not.toHaveBeenCalled()