fix(tests): isolate skills homes and stabilize reload assertions (#2307)

Co-authored-by: OpenClaude CI Maintenance <ci@example.invalid>

src/skills/loadSkillsDir.test.ts

@@ -5,6 +5,7 @@ import {
   mkdirSync,
   readdirSync,
   readFileSync,
+  realpathSync,
   rmSync,
   statSync,
   symlinkSync,
@@ -131,17 +132,32 @@ function setRealFilesystemForTest(): ReturnType<typeof getFsImplementation> {
   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<void>[] = []
+      const changedPaths = new Set<string>()
       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()

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<string, string | undefined>
 
 function installMocks(): void {
   clearCommandsCache = mock(() => {})
@@ -91,8 +94,29 @@ function sleep(ms: number): Promise<void> {
   return new Promise(resolve => setTimeout(resolve, ms))
 }
 
+async function advanceTime(ms: number): Promise<void> {
+  await new Promise<void>(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<void>(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<string>()
     detector._setDependenciesForTesting({
       getSkillsPath: () => own,
       getAdditionalDirectoriesForClaudeMd: () => [],
@@ -376,6 +408,7 @@ describe('skillChangeDetector reload batching', () => {
       resetSentSkillNames,
       watch: (paths: string[], options: Parameters<typeof chokidar.watch>[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()