diff --git a/cli/src/services/doctor-service.ts b/cli/src/services/doctor-service.ts index 1837fea4..0ebd5add 100644 --- a/cli/src/services/doctor-service.ts +++ b/cli/src/services/doctor-service.ts @@ -1,4 +1,4 @@ -import { readdir, writeFile, readFile } from 'node:fs/promises' +import { lstat, readdir, writeFile, readFile } from 'node:fs/promises' import { join } from 'node:path' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' @@ -98,7 +98,18 @@ async function scanMetadata(cwd: string, skipped: DoctorResult['skipped']): Prom for (const dirName of topEntries) { if (!dirName.startsWith('.')) continue - const skillsDir = join(cwd, dirName, 'skills') + const agentDir = join(cwd, dirName) + try { + const st = await lstat(agentDir) + if (st.isSymbolicLink() || !st.isDirectory()) { + skipped.push({ path: agentDir, reason: 'not a regular directory' }) + continue + } + } catch { + skipped.push({ path: agentDir, reason: 'cannot stat' }) + continue + } + const skillsDir = join(agentDir, 'skills') let slugDirs: string[] try { slugDirs = await readdir(skillsDir) @@ -107,17 +118,39 @@ async function scanMetadata(cwd: string, skipped: DoctorResult['skipped']): Prom } for (const slug of slugDirs) { - const metadataPath = join(skillsDir, slug, '.skillhub', 'metadata.json') + const slugPath = join(skillsDir, slug) + try { + const st = await lstat(slugPath) + if (st.isSymbolicLink() || !st.isDirectory()) { + skipped.push({ path: slugPath, reason: 'not a regular directory' }) + continue + } + } catch { + skipped.push({ path: slugPath, reason: 'cannot stat' }) + continue + } + const skillhubDir = join(slugPath, '.skillhub') + try { + const skillhubSt = await lstat(skillhubDir) + if (skillhubSt.isSymbolicLink() || !skillhubSt.isDirectory()) { + skipped.push({ path: slugPath, reason: '.skillhub is not a regular directory' }) + continue + } + } catch { + skipped.push({ path: slugPath, reason: 'no .skillhub directory' }) + continue + } + const metadataPath = join(skillhubDir, 'metadata.json') try { const content = await readFile(metadataPath, 'utf-8') const metadata = JSON.parse(content) as MetadataJson if (!metadata.registry || !metadata.namespace || !metadata.slug || !metadata.version || !metadata.agent || !metadata.installedAt) { - skipped.push({ path: join(skillsDir, slug), reason: 'incomplete metadata' }) + skipped.push({ path: slugPath, reason: 'incomplete metadata' }) continue } - results.push({ metadata, installDir: join(skillsDir, slug) }) + results.push({ metadata, installDir: slugPath }) } catch { - skipped.push({ path: join(skillsDir, slug), reason: 'no .skillhub/metadata.json' }) + skipped.push({ path: slugPath, reason: 'no .skillhub/metadata.json' }) } } } diff --git a/cli/src/stores/inventory-store.ts b/cli/src/stores/inventory-store.ts index 6dc49c17..447c6e32 100644 --- a/cli/src/stores/inventory-store.ts +++ b/cli/src/stores/inventory-store.ts @@ -1,4 +1,4 @@ -import { readFile, rename, writeFile } from 'node:fs/promises' +import { open, readFile, rename, rm, writeFile } from 'node:fs/promises' import { dirname } from 'node:path' import { joinPath, userStateDir, ensureDir, pathExists } from '../platform/paths' @@ -42,10 +42,74 @@ export class InventoryStore { await ensureDir(dirname(this.path)) const payload = JSON.stringify(inventory, null, 2) JSON.parse(payload) + + const lockPath = `${this.path}.lock` const tmpPath = `${this.path}.${process.pid}.${Date.now()}.tmp` - await writeFile(tmpPath, payload) - JSON.parse(await readFile(tmpPath, 'utf-8')) - await rename(tmpPath, this.path) + + let lockHandle: Awaited> | null = null + try { + // Acquire exclusive lock with retry and stale lock detection + lockHandle = await this.acquireLock(lockPath) + + await writeFile(tmpPath, payload) + JSON.parse(await readFile(tmpPath, 'utf-8')) + await rename(tmpPath, this.path) + } finally { + // Clean up temp file if it still exists + await rm(tmpPath, { force: true }).catch(() => {}) + + // Release lock + if (lockHandle) { + await lockHandle.close().catch(() => {}) + await rm(lockPath, { force: true }).catch(() => {}) + } + } + } + + private async acquireLock(lockPath: string, maxRetries = 10, retryDelayMs = 100): Promise>> { + for (let attempt = 0; attempt < maxRetries; attempt++) { + try { + // Try to create lock file with PID and timestamp + const lockHandle = await open(lockPath, 'wx') + const lockData = JSON.stringify({ pid: process.pid, timestamp: Date.now() }) + await writeFile(lockPath, lockData) + return lockHandle + } catch (err: any) { + if (err.code !== 'EEXIST') throw err + + // Lock exists, check if it's stale (older than 30 seconds) + // 30s threshold chosen to balance between: + // - Allowing slow operations to complete (e.g., large inventory writes) + // - Recovering quickly from crashed processes + try { + const lockContent = await readFile(lockPath, 'utf-8') + const lockData = JSON.parse(lockContent) as { pid: number; timestamp: number } + const ageMs = Date.now() - lockData.timestamp + + if (ageMs > 30000) { + // Stale lock detected - verify the process is actually dead + try { + // process.kill(pid, 0) throws if process doesn't exist + process.kill(lockData.pid, 0) + // Process still alive, wait and retry + } catch { + // Process is dead, safe to remove stale lock + await rm(lockPath, { force: true }).catch(() => {}) + continue + } + } + } catch { + // Lock file disappeared or corrupted, retry + continue + } + + // Lock is held by another active process, wait and retry with exponential backoff + if (attempt < maxRetries - 1) { + await new Promise(resolve => setTimeout(resolve, retryDelayMs * Math.pow(2, attempt))) + } + } + } + throw new Error(`Failed to acquire lock after ${maxRetries} attempts`) } async upsertTarget( @@ -70,7 +134,7 @@ export class InventoryStore { } else { item.targets.push(target) } - await this.write(inventory) + await this.writeAtomic(inventory) } async removeTarget(registry: string, namespace: string, slug: string, installDir: string): Promise { @@ -83,7 +147,7 @@ export class InventoryStore { if (item.targets.length === 0) { inventory.items = inventory.items.filter(i => i !== item) } - await this.write(inventory) + await this.writeAtomic(inventory) return true } @@ -97,7 +161,7 @@ export class InventoryStore { } if (removed > 0) { inventory.items = inventory.items.filter(item => item.targets.length > 0) - await this.write(inventory) + await this.writeAtomic(inventory) } return removed } diff --git a/cli/test/unit/services/doctor-service.test.ts b/cli/test/unit/services/doctor-service.test.ts index be17b05c..7272d6d0 100644 --- a/cli/test/unit/services/doctor-service.test.ts +++ b/cli/test/unit/services/doctor-service.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test' -import { mkdtemp, mkdir, writeFile } from 'node:fs/promises' +import { mkdtemp, mkdir, writeFile, symlink } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { runDoctor } from '../../../src/services/doctor-service' @@ -42,7 +42,7 @@ describe('doctor-service', () => { const result = await runDoctor(cwd, home) expect(result.itemsRestored).toBe(0) expect(result.skipped).toHaveLength(1) - expect(result.skipped[0]!.reason).toBe('no .skillhub/metadata.json') + expect(result.skipped[0]!.reason).toBe('no .skillhub directory') }) test('detects version conflicts', async () => { @@ -64,4 +64,61 @@ describe('doctor-service', () => { expect(result.conflicts[0]!.versions).toContain('1.0.0') expect(result.conflicts[0]!.versions).toContain('2.0.0') }) + + test('skips symlinked skill directories', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'doctor-test-')) + const home = await mkdtemp(join(tmpdir(), 'doctor-home-')) + + await setupSkillDir(cwd, '.codex', 'real-skill', { + registry: 'https://skill.xfyun.cn', namespace: 'global', slug: 'real-skill', + version: '1.0.0', agent: 'codex', installedAt: '2026-04-20T12:00:00Z' + }) + + const skillsDir = join(cwd, '.codex', 'skills') + const realDir = join(skillsDir, 'real-skill') + await symlink(realDir, join(skillsDir, 'symlink-skill')) + + const result = await runDoctor(cwd, home) + expect(result.itemsRestored).toBe(1) + expect(result.skipped.some(s => s.reason === 'not a regular directory')).toBe(true) + }) + + test('skips symlinked agent directories', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'doctor-test-')) + const home = await mkdtemp(join(tmpdir(), 'doctor-home-')) + + await setupSkillDir(cwd, '.codex', 'pdf-parser', { + registry: 'https://skill.xfyun.cn', namespace: 'global', slug: 'pdf-parser', + version: '1.0.0', agent: 'codex', installedAt: '2026-04-20T12:00:00Z' + }) + + await symlink(join(cwd, '.codex'), join(cwd, '.fake-agent')) + + const result = await runDoctor(cwd, home) + expect(result.itemsRestored).toBe(1) + expect(result.targetsRestored).toBe(1) + }) + + test('skips symlinked .skillhub directories', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'doctor-test-')) + const home = await mkdtemp(join(tmpdir(), 'doctor-home-')) + + const skillDir = join(cwd, '.codex', 'skills', 'evil-skill') + await mkdir(skillDir, { recursive: true }) + + const realMetaDir = await mkdtemp(join(tmpdir(), 'real-meta-')) + await writeFile(join(realMetaDir, 'metadata.json'), JSON.stringify({ + registry: 'https://skill.xfyun.cn', + namespace: 'global', + slug: 'evil-skill', + version: '1.0.0', + agent: 'codex', + installedAt: '2026-04-20T12:00:00Z' + })) + await symlink(realMetaDir, join(skillDir, '.skillhub')) + + const result = await runDoctor(cwd, home) + expect(result.itemsRestored).toBe(0) + expect(result.skipped.some(s => s.reason === '.skillhub is not a regular directory')).toBe(true) + }) })