Skip to content

Commit 6c80120

Browse files
committed
fix: address copilot review comments on pipeline and compat
- Remove !paused guard in checkBackpressure to fix backpressure chaining - Sort resolved entries before hashing for stable lockDigest - Add skipResolve: true in add.ts runInstallPipeline to avoid double resolve - Unify installDir derivation across fetchQueue and pipeline - Add concurrency-safe in-flight Promise map to cache.getOrSet - Restore !important in responsive CSS hover/transform resets - Restore backward-compatible exports (installSkills, fetchSkillsFromLock, linkSkillsFromLock) - Remove unused downloadRoot / mkdtemp leak in downloadNpmPackageTarball - Remove stat-only fast path in isLockInSync to avoid false positives - Validate linkTargets symlinks in pipeline up-to-date fast path - Ensure installDir exists before writing lock copy - Remove unused cleanupPackedNpmPackage import - Update architecture docs to reflect install resolve-then-pipeline behavior
1 parent 7a47f1f commit 6c80120

13 files changed

Lines changed: 253 additions & 39 deletions

File tree

packages/skills-package-manager/src/commands/add.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,8 @@ async function runInstallPipeline(cwd: string) {
5454
const runtimeLock = ctx.lockfile
5555
? await withBundledSelfSkillLock(cwd, ctx.manifest, ctx.lockfile)
5656
: null
57-
await runPipeline({ ctx, entries: runtimeLock?.skills ?? ctx.lockfile?.skills ?? {} })
57+
const entries = runtimeLock?.skills ?? ctx.lockfile?.skills ?? {}
58+
await runPipeline({ ctx, entries, skipResolve: true })
5859
}
5960

6061
function buildLinkSpecifier(sourceRoot: string, skillPath: string): string {

packages/skills-package-manager/src/commands/install.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { readFile, writeFile } from 'node:fs/promises'
1+
import { mkdir, readFile, writeFile } from 'node:fs/promises'
22
import path from 'node:path'
33
import YAML from 'yaml'
44
import { isLockInSync, isSkillsLockEqual } from '../config/compareSkillsLock'
@@ -28,6 +28,7 @@ async function writeInstallDirLock(
2828
const dirPath = path.join(cwd, installDir)
2929
const filePath = path.join(dirPath, 'lock.yaml')
3030
try {
31+
await mkdir(dirPath, { recursive: true })
3132
await writeFile(filePath, YAML.stringify(lockfile), 'utf8')
3233
} catch (error) {
3334
throw new Error(`Failed to write install-dir lock copy: ${(error as Error).message}`)

packages/skills-package-manager/src/config/compareSkillsLock.ts

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -113,16 +113,6 @@ export async function isLockInSync(
113113
): Promise<boolean> {
114114
if (!lock) return false
115115

116-
// Fast path: if manifest file hasn't changed since last install, skip full check
117-
if (
118-
manifestStat &&
119-
installState?.manifestStat &&
120-
manifestStat.mtimeMs === installState.manifestStat.mtimeMs &&
121-
manifestStat.size === installState.manifestStat.size
122-
) {
123-
return true
124-
}
125-
126116
if (normalizeInstallDir(manifest.installDir) !== normalizeInstallDir(lock.installDir)) {
127117
return false
128118
}

packages/skills-package-manager/src/fetchers/npm.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import path from 'node:path'
22
import type { SkillsLockEntry } from '../config/types'
33
import { materializePackedSkill } from '../install/materializePackedSkill'
4-
import { cleanupPackedNpmPackage, downloadNpmPackageTarball } from '../npm/packPackage'
4+
import { downloadNpmPackageTarball } from '../npm/packPackage'
55
import type { CacheManager } from '../pipeline/types'
66

77
// In-flight deduplication for concurrent npm fetches (memory-level, faster than filesystem cache)
@@ -57,9 +57,10 @@ export async function fetchNpmSkill(
5757
return { installPath: path.join(rootDir, installDir, skillName), fromCache }
5858
} finally {
5959
inFlightDownloads.delete(cacheKey)
60-
// Note: cleanupPackedNpmPackage is not called here because the tarball may still be
61-
// needed by other concurrent in-flight consumers. In the old installSkills flow,
62-
// cleanup happened at the end of fetchSkillsFromLock after all consumers settled.
63-
// For now we rely on the OS to clean temp files, or a future persistent cache layer.
60+
// Note: cleanupPackedNpmPackage is not called here because tarballs are stored
61+
// in a persistent cache directory (see downloadNpmPackageTarball). The old
62+
// installSkills flow cleaned up temp directories at the end of fetchSkillsFromLock,
63+
// but the current implementation uses a deterministic persistent cache, so no
64+
// per-run cleanup is required.
6465
}
6566
}

packages/skills-package-manager/src/index.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,13 @@ export {
5858
parseOwnerRepo,
5959
} from './github/listSkills'
6060
export type { SkillInfo } from './github/types'
61-
export { createInstallProgressReporter } from './install/progressReporter'
6261
// Install
62+
export {
63+
fetchSkillsFromLock,
64+
installSkills,
65+
linkSkillsFromLock,
66+
} from './install/installSkills'
67+
export { createInstallProgressReporter } from './install/progressReporter'
6368
export { installStageHooks, withBundledSelfSkillLock } from './install/withBundledSelfSkillLock'
6469
// Specifiers
6570
export { normalizeSpecifier } from './specifiers/normalizeSpecifier'
Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
import { access } from 'node:fs/promises'
2+
import path from 'node:path'
3+
import { isLockInSync } from '../config/compareSkillsLock'
4+
import { readSkillsLock } from '../config/readSkillsLock'
5+
import { readSkillsManifest } from '../config/readSkillsManifest'
6+
import {
7+
getBundledSelfSkillSpecifier,
8+
SELF_SKILL_NAME,
9+
shouldInjectBundledSelfSkill,
10+
} from '../config/skillsManifest'
11+
import { resolveLockEntry, syncSkillsLock } from '../config/syncSkillsLock'
12+
import type { InstallProgressListener, SkillsLock, SkillsManifest } from '../config/types'
13+
import { writeSkillsLock } from '../config/writeSkillsLock'
14+
import { fetchSkill } from '../fetchers'
15+
import { applySkillPatch } from '../patches/skillPatch'
16+
import { createFileSystemCache } from '../pipeline/cache'
17+
import { sha256 } from '../utils/hash'
18+
import { readInstallState, writeInstallState } from './installState'
19+
import { linkSkill } from './links'
20+
import { pruneManagedSkills } from './pruneManagedSkills'
21+
22+
async function areManagedSkillsInstalled(
23+
rootDir: string,
24+
installDir: string,
25+
skillNames: string[],
26+
): Promise<boolean> {
27+
for (const skillName of skillNames) {
28+
try {
29+
await access(path.join(rootDir, installDir, skillName, 'SKILL.md'))
30+
} catch {
31+
return false
32+
}
33+
}
34+
return true
35+
}
36+
37+
export async function withBundledSelfSkillLock(
38+
rootDir: string,
39+
manifest: SkillsManifest,
40+
lockfile: SkillsLock,
41+
): Promise<SkillsLock> {
42+
if (!shouldInjectBundledSelfSkill(manifest) || lockfile.skills[SELF_SKILL_NAME]) {
43+
return lockfile
44+
}
45+
46+
const { entry } = await resolveLockEntry(rootDir, getBundledSelfSkillSpecifier(), SELF_SKILL_NAME)
47+
48+
return {
49+
...lockfile,
50+
skills: {
51+
...lockfile.skills,
52+
[SELF_SKILL_NAME]: entry,
53+
},
54+
}
55+
}
56+
57+
export async function fetchSkillsFromLock(
58+
rootDir: string,
59+
manifest: SkillsManifest,
60+
lockfile: SkillsLock,
61+
options?: {
62+
onProgress?: InstallProgressListener
63+
},
64+
) {
65+
const installDir = manifest.installDir ?? '.agents/skills'
66+
const linkTargets = manifest.linkTargets ?? []
67+
68+
await pruneManagedSkills(rootDir, installDir, linkTargets, Object.keys(lockfile.skills))
69+
70+
const lockDigest = sha256(JSON.stringify(lockfile))
71+
const state = await readInstallState(rootDir, installDir)
72+
if (
73+
state?.lockDigest === lockDigest &&
74+
(await areManagedSkillsInstalled(rootDir, installDir, Object.keys(lockfile.skills)))
75+
) {
76+
return { status: 'skipped', reason: 'up-to-date' } as const
77+
}
78+
79+
const cache = createFileSystemCache(rootDir)
80+
81+
try {
82+
for (const [skillName, entry] of Object.entries(lockfile.skills)) {
83+
const { installPath } = await fetchSkill(rootDir, skillName, entry, installDir, cache)
84+
if (entry.patch) {
85+
await applySkillPatch(installPath, path.resolve(rootDir, entry.patch.path))
86+
}
87+
options?.onProgress?.({ type: 'added', skillName })
88+
}
89+
90+
await writeInstallState(rootDir, installDir, {
91+
lockDigest,
92+
installDir,
93+
linkTargets,
94+
installerVersion: '0.1.0',
95+
installedAt: new Date().toISOString(),
96+
})
97+
} catch (error) {
98+
throw error
99+
}
100+
101+
return { status: 'fetched', fetched: Object.keys(lockfile.skills) } as const
102+
}
103+
104+
export async function linkSkillsFromLock(
105+
rootDir: string,
106+
manifest: SkillsManifest,
107+
lockfile: SkillsLock,
108+
options?: {
109+
onProgress?: InstallProgressListener
110+
},
111+
) {
112+
const installDir = manifest.installDir ?? '.agents/skills'
113+
const linkTargets = manifest.linkTargets ?? []
114+
115+
for (const skillName of Object.keys(lockfile.skills)) {
116+
for (const linkTarget of linkTargets) {
117+
await linkSkill(rootDir, installDir, linkTarget, skillName)
118+
}
119+
options?.onProgress?.({ type: 'installed', skillName })
120+
}
121+
122+
return { status: 'linked', linked: Object.keys(lockfile.skills) } as const
123+
}
124+
125+
export async function installSkills(
126+
rootDir: string,
127+
options?: { frozenLockfile?: boolean; onProgress?: InstallProgressListener },
128+
) {
129+
const manifest = await readSkillsManifest(rootDir)
130+
if (!manifest) {
131+
return { status: 'skipped', reason: 'manifest-missing' } as const
132+
}
133+
134+
const currentLock = await readSkillsLock(rootDir)
135+
136+
let lockfile: SkillsLock
137+
138+
if (options?.frozenLockfile) {
139+
if (!currentLock) {
140+
throw new Error('Lockfile is required in frozen mode but none was found')
141+
}
142+
if (!(await isLockInSync(rootDir, manifest, currentLock))) {
143+
throw new Error(
144+
'Lockfile is out of sync with manifest. Run install without --frozen-lockfile to update.',
145+
)
146+
}
147+
lockfile = currentLock
148+
for (const skillName of Object.keys(lockfile.skills)) {
149+
options?.onProgress?.({ type: 'resolved', skillName })
150+
}
151+
} else {
152+
lockfile = await syncSkillsLock(rootDir, manifest, currentLock, {
153+
onProgress: options?.onProgress,
154+
})
155+
}
156+
157+
const runtimeLock = await withBundledSelfSkillLock(rootDir, manifest, lockfile)
158+
159+
await fetchSkillsFromLock(rootDir, manifest, runtimeLock, {
160+
onProgress: options?.onProgress,
161+
})
162+
await linkSkillsFromLock(rootDir, manifest, runtimeLock, {
163+
onProgress: options?.onProgress,
164+
})
165+
166+
if (!options?.frozenLockfile) {
167+
await writeSkillsLock(rootDir, lockfile)
168+
}
169+
170+
return { status: 'installed', installed: Object.keys(runtimeLock.skills) } as const
171+
}

packages/skills-package-manager/src/npm/packPackage.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -360,9 +360,7 @@ export async function downloadNpmPackageTarball(
360360
// Cache miss
361361
}
362362

363-
// 2. Download to temp dir
364-
const downloadRoot = await mkdtemp(path.join(tmpdir(), 'skills-pm-npm-download-'))
365-
363+
// 2. Download into memory and write to persistent cache
366364
try {
367365
const config = await loadNpmConfig(cwd)
368366
const response = await fetch(tarballUrl, {
@@ -382,7 +380,6 @@ export async function downloadNpmPackageTarball(
382380
await writeFile(cachePath, tarballBuffer)
383381
return { tarballPath: cachePath, fromCache: false }
384382
} catch (error) {
385-
await rm(downloadRoot, { recursive: true, force: true }).catch(() => {})
386383
throw new Error(`Failed to download npm tarball ${tarballUrl}: ${(error as Error).message}`, {
387384
cause: error as Error,
388385
})

packages/skills-package-manager/src/pipeline/cache.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ const CACHE_DIR = '.skills-pm-cache'
77

88
export function createFileSystemCache(cwd: string): CacheManager {
99
const cacheRoot = path.join(cwd, CACHE_DIR)
10+
const inFlight = new Map<string, Promise<string>>()
1011

1112
async function ensureCache() {
1213
await mkdir(cacheRoot, { recursive: true })
@@ -40,9 +41,21 @@ export function createFileSystemCache(cwd: string): CacheManager {
4041
if (cached !== null) {
4142
return cached as T
4243
}
43-
const value = await factory()
44-
await this.set(key, value)
45-
return value
44+
45+
let inflight = inFlight.get(key)
46+
if (inflight) {
47+
return (await inflight) as T
48+
}
49+
50+
inflight = factory()
51+
inFlight.set(key, inflight)
52+
try {
53+
const value = await inflight
54+
await this.set(key, value)
55+
return value as T
56+
} finally {
57+
inFlight.delete(key)
58+
}
4659
},
4760
}
4861
}

packages/skills-package-manager/src/pipeline/fetchQueue.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export function createFetchTaskQueue(
4848
bus: PipelineBus,
4949
options: { concurrency: number; maxPending?: number },
5050
): FetchQueue {
51-
const installDir = ctx.manifest.installDir ?? '.agents/skills'
51+
const installDir = ctx.lockfile?.installDir ?? ctx.manifest.installDir ?? '.agents/skills'
5252

5353
async function processor(task: FetchTask): Promise<FetchResult> {
5454
if (await isSkillUpToDate(ctx.cwd, installDir, task.skillName, task.entry)) {

packages/skills-package-manager/src/pipeline/index.ts

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { access } from 'node:fs/promises'
1+
import { access, lstat, readlink } from 'node:fs/promises'
22
import path from 'node:path'
33
import type { SkillsLock, SkillsLockEntry } from '../config/types'
44
import { ErrorCode, getErrorMessage, SpmError } from '../errors'
@@ -34,6 +34,31 @@ async function areManagedSkillsInstalled(
3434
return true
3535
}
3636

37+
async function areLinksUpToDate(
38+
rootDir: string,
39+
installDir: string,
40+
linkTargets: string[],
41+
skillNames: string[],
42+
): Promise<boolean> {
43+
for (const linkTarget of linkTargets) {
44+
const targetDir = path.resolve(rootDir, linkTarget)
45+
for (const skillName of skillNames) {
46+
try {
47+
const linkPath = path.join(targetDir, skillName)
48+
const stats = await lstat(linkPath)
49+
if (!stats.isSymbolicLink()) return false
50+
const target = await readlink(linkPath)
51+
const resolvedTarget = path.resolve(path.dirname(linkPath), target)
52+
const expectedTarget = path.resolve(rootDir, installDir, skillName)
53+
if (resolvedTarget !== expectedTarget) return false
54+
} catch {
55+
return false
56+
}
57+
}
58+
}
59+
return true
60+
}
61+
3762
export async function runPipeline(input: RunPipelineInput): Promise<PipelineResult> {
3863
const { ctx, entries, skipResolve = false, options = {} } = input
3964
const bus = createPipelineBus(options.onProgress)
@@ -43,19 +68,24 @@ export async function runPipeline(input: RunPipelineInput): Promise<PipelineResu
4368
const linkTargets = ctx.lockfile?.linkTargets ?? ctx.manifest.linkTargets ?? []
4469

4570
// Fast path: skip all work when install state is up-to-date
71+
const sortedSkillNames = Object.keys(entries).sort()
72+
const sortedEntries = Object.fromEntries(
73+
sortedSkillNames.map((skillName) => [skillName, entries[skillName]]),
74+
) as Record<string, SkillsLockEntry>
4675
const lockfileForDigest: SkillsLock = {
4776
lockfileVersion: '0.1',
4877
installDir,
4978
linkTargets,
50-
skills: entries,
79+
skills: sortedEntries,
5180
}
5281
const currentDigest = sha256(JSON.stringify(lockfileForDigest))
5382
if (
5483
ctx.installState?.lockDigest === currentDigest &&
55-
(await areManagedSkillsInstalled(ctx.cwd, installDir, Object.keys(entries)))
84+
(await areManagedSkillsInstalled(ctx.cwd, installDir, sortedSkillNames)) &&
85+
(await areLinksUpToDate(ctx.cwd, installDir, linkTargets, sortedSkillNames))
5686
) {
5787
// Emit progress events so callers (e.g. the CLI reporter) see consistent output
58-
for (const skillName of Object.keys(entries)) {
88+
for (const skillName of sortedSkillNames) {
5989
bus.emitLinked({ skillName })
6090
}
6191
return bus.getResults()
@@ -167,7 +197,12 @@ export async function runPipeline(input: RunPipelineInput): Promise<PipelineResu
167197
linkTargets,
168198
skills: skipResolve
169199
? entries
170-
: Object.fromEntries(results.resolved.map((r) => [r.skillName, r.entry])),
200+
: Object.fromEntries(
201+
results.resolved
202+
.slice()
203+
.sort((a, b) => a.skillName.localeCompare(b.skillName))
204+
.map((r) => [r.skillName, r.entry]),
205+
),
171206
}
172207
const lockDigest = sha256(JSON.stringify(lockfile))
173208
await writeInstallState(ctx.cwd, installDir, {

0 commit comments

Comments
 (0)