Skip to content

Commit 4ff411d

Browse files
committed
Preserve reviewed skill install hardening after rebase
Rebasing PR #1162 onto current main flattened an earlier merge commit that carried reviewed Skill Hub hardening and regression coverage. This restores those final-tree changes as a normal linear commit so the rebased PR keeps the same behavior reviewers approved without retaining merge commits or mainline noise. Constraint: Keep the PR branch linear for maintainer review while preserving the reviewed final tree from the conflict-resolved integration branch. Rejected: Push the plain rebase result | it would drop registry sha256/version/trust metadata handling and associated tests from the reviewed PR state. Confidence: high Scope-risk: narrow Directive: Do not remove the registry sha256 requirement or install-path regression tests without another security review. Tested: final tree compared against fix-pr-1162-conflicts before verification
1 parent 2e1bb06 commit 4ff411d

4 files changed

Lines changed: 184 additions & 17 deletions

File tree

src/cli/handlers/skills.test.ts

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,8 @@ test.serial('installs a registry skill by id from a local registry file', async
384384
path: 'skills/sample-skill/SKILL.md',
385385
homepage: 'https://github.com/Gitlawb/openclaude-skills/tree/main/skills/sample-skill',
386386
sha256: sha256OfSkillSource(VALID_SKILL),
387+
min_openclaude_version: '0.1.0',
388+
tools_required: ['Read', 'Bash'],
387389
},
388390
]),
389391
'utf8',
@@ -399,9 +401,86 @@ test.serial('installs a registry skill by id from a local registry file', async
399401
join(cwd, '.openclaude', 'skills', 'sample-skill', 'skill.json'),
400402
'utf8',
401403
),
402-
) as { trust: string; sha256: string }
404+
) as {
405+
trust: string
406+
sha256: string
407+
min_openclaude_version: string
408+
tools_required: string[]
409+
}
403410
assert.equal(installedMetadata.trust, 'official')
404411
assert.equal(installedMetadata.sha256, sha256OfSkillSource(VALID_SKILL))
412+
assert.equal(installedMetadata.min_openclaude_version, '0.1.0')
413+
assert.deepEqual(installedMetadata.tools_required, ['Read', 'Bash'])
414+
})
415+
})
416+
417+
test.serial('rejects registry skills without a sha256 pin', async () => {
418+
await withTempDir(async tempDir => {
419+
const cwd = join(tempDir, 'project')
420+
const sourceDir = writeSkillDir(join(tempDir, 'registry-source'))
421+
const registryPath = join(tempDir, 'registry.json')
422+
mkdirSync(cwd, { recursive: true })
423+
writeFileSync(
424+
registryPath,
425+
JSON.stringify([
426+
{
427+
id: 'gitlawb/sample-skill',
428+
name: 'sample-skill',
429+
title: 'Sample Skill',
430+
description: 'Sample skill used by install tests.',
431+
trust: 'official',
432+
version: '0.1.0',
433+
license: 'MIT',
434+
author: 'OpenClaude Tests',
435+
source: join(sourceDir, 'SKILL.md'),
436+
},
437+
]),
438+
'utf8',
439+
)
440+
441+
await skillsInstallHandler('sample-skill', {
442+
projectDir: cwd,
443+
registry: registryPath,
444+
})
445+
446+
assert.equal(process.exitCode, 1)
447+
assert.equal(existsSync(join(cwd, '.openclaude', 'skills')), false)
448+
})
449+
})
450+
451+
test.serial('rejects registry skills that require a newer OpenClaude version', async () => {
452+
await withTempDir(async tempDir => {
453+
const cwd = join(tempDir, 'project')
454+
const sourceDir = writeSkillDir(join(tempDir, 'registry-source'))
455+
const registryPath = join(tempDir, 'registry.json')
456+
mkdirSync(cwd, { recursive: true })
457+
writeFileSync(
458+
registryPath,
459+
JSON.stringify([
460+
{
461+
id: 'gitlawb/sample-skill',
462+
name: 'sample-skill',
463+
title: 'Sample Skill',
464+
description: 'Sample skill used by install tests.',
465+
trust: 'official',
466+
version: '0.1.0',
467+
license: 'MIT',
468+
author: 'OpenClaude Tests',
469+
source: join(sourceDir, 'SKILL.md'),
470+
sha256: sha256OfSkillSource(VALID_SKILL),
471+
min_openclaude_version: '999.0.0',
472+
},
473+
]),
474+
'utf8',
475+
)
476+
477+
await skillsInstallHandler('sample-skill', {
478+
projectDir: cwd,
479+
registry: registryPath,
480+
})
481+
482+
assert.equal(process.exitCode, 1)
483+
assert.equal(existsSync(join(cwd, '.openclaude', 'skills')), false)
405484
})
406485
})
407486

src/cli/handlers/skillsInstall.ts

Lines changed: 87 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@ import { createHash } from 'crypto'
22
import { cp, mkdir, mkdtemp, readFile, rm, stat, writeFile } from 'fs/promises'
33
import { tmpdir } from 'os'
44
import { basename, dirname, isAbsolute, join, relative, resolve } from 'path'
5+
import { coerce, lt } from 'semver'
56
import { getCwd } from '../../utils/cwd.js'
67
import { getClaudeConfigHomeDir } from '../../utils/envUtils.js'
78
import { getDisplayPath } from '../../utils/file.js'
89
import { parseFrontmatter } from '../../utils/frontmatterParser.js'
10+
import { publicBuildVersion } from '../../utils/version.js'
911
import { validateSkillPath } from './skillsValidation.js'
1012

1113
export type InstallOptions = {
@@ -28,6 +30,8 @@ type SkillRegistryEntry = {
2830
path?: unknown
2931
homepage?: unknown
3032
sha256?: unknown
33+
min_openclaude_version?: unknown
34+
tools_required?: unknown
3135
category?: unknown
3236
tags?: unknown
3337
author?: unknown
@@ -147,6 +151,8 @@ function registryMetadata(entry: SkillRegistryEntry): Record<string, unknown> {
147151
'path',
148152
'homepage',
149153
'sha256',
154+
'min_openclaude_version',
155+
'tools_required',
150156
] as const) {
151157
const value = entry[key]
152158
if (value !== undefined) metadata[key] = value
@@ -159,6 +165,63 @@ function sha256OfSkillSource(text: string): string {
159165
return createHash('sha256').update(normalized, 'utf8').digest('hex')
160166
}
161167

168+
function stringArray(value: unknown): string[] {
169+
if (!Array.isArray(value)) return []
170+
return value.filter((item): item is string => typeof item === 'string' && item.trim() !== '')
171+
}
172+
173+
function requireRegistrySha256(entry: SkillRegistryEntry, spec: string): string {
174+
if (typeof entry.sha256 !== 'string' || entry.sha256.trim() === '') {
175+
throw new Error(
176+
`Registry entry "${spec}" is missing sha256. Refusing to install an unpinned skill.`,
177+
)
178+
}
179+
return entry.sha256.trim()
180+
}
181+
182+
function assertCompatibleOpenClaudeVersion(entry: SkillRegistryEntry, spec: string): string | undefined {
183+
if (
184+
typeof entry.min_openclaude_version !== 'string' ||
185+
entry.min_openclaude_version.trim() === ''
186+
) {
187+
return undefined
188+
}
189+
190+
const minimum = entry.min_openclaude_version.trim()
191+
const current = coerce(publicBuildVersion)
192+
const required = coerce(minimum)
193+
194+
if (!current || !required) {
195+
throw new Error(
196+
`Registry entry "${spec}" has an invalid min_openclaude_version value: ${minimum}.`,
197+
)
198+
}
199+
200+
if (lt(current, required)) {
201+
throw new Error(
202+
`Skill "${spec}" requires OpenClaude ${required.version} or newer. Current version is ${current.version}.`,
203+
)
204+
}
205+
206+
return minimum
207+
}
208+
209+
function trustInstallWarning(trust: string): string | null {
210+
if (trust === 'official') {
211+
return null
212+
}
213+
if (trust === 'verified') {
214+
return 'Warning: this verified community skill was reviewed, but is not maintained as an official OpenClaude skill.'
215+
}
216+
if (trust === 'community') {
217+
return 'Warning: this community skill passed registry validation, but may not be deeply reviewed or maintained by OpenClaude maintainers.'
218+
}
219+
if (trust === 'deprecated') {
220+
return 'Warning: this skill is marked deprecated. Install only if you intentionally need this older workflow.'
221+
}
222+
return `Warning: this skill has trust tier "${trust}". Review SKILL.md before using it.`
223+
}
224+
162225
function getSkillNameFromMarkdown(markdown: string, fallback: string): string {
163226
try {
164227
const { frontmatter } = parseFrontmatter(markdown, 'SKILL.md')
@@ -265,6 +328,8 @@ async function prepareInstallCandidate(
265328
skillName: string
266329
sourceDescription: string
267330
trust: string
331+
toolsRequired: string[]
332+
minOpenClaudeVersion?: string
268333
}> {
269334
if (!isUrl(spec) && (await pathExists(resolve(spec)))) {
270335
const sourcePath = resolve(spec)
@@ -287,6 +352,7 @@ async function prepareInstallCandidate(
287352
skillName,
288353
sourceDescription: getDisplayPath(sourcePath),
289354
trust: 'local',
355+
toolsRequired: [],
290356
}
291357
}
292358

@@ -297,6 +363,7 @@ async function prepareInstallCandidate(
297363
...prepared,
298364
sourceDescription: getDisplayPath(sourcePath),
299365
trust: 'local',
366+
toolsRequired: [],
300367
}
301368
}
302369

@@ -308,6 +375,7 @@ async function prepareInstallCandidate(
308375
...prepared,
309376
sourceDescription: spec,
310377
trust: 'url',
378+
toolsRequired: [],
311379
}
312380
}
313381

@@ -316,14 +384,14 @@ async function prepareInstallCandidate(
316384
throw new Error(`Skill "${spec}" was not found in the registry.`)
317385
}
318386

387+
const expectedSha256 = requireRegistrySha256(entry, spec)
388+
const minOpenClaudeVersion = assertCompatibleOpenClaudeVersion(entry, spec)
319389
const markdown = await readSourceText(entry.source)
320-
if (typeof entry.sha256 === 'string') {
321-
const actual = sha256OfSkillSource(markdown)
322-
if (actual !== entry.sha256) {
323-
throw new Error(
324-
`Registry checksum mismatch for "${spec}". Expected ${entry.sha256}, got ${actual}.`,
325-
)
326-
}
390+
const actual = sha256OfSkillSource(markdown)
391+
if (actual !== expectedSha256) {
392+
throw new Error(
393+
`Registry checksum mismatch for "${spec}". Expected ${expectedSha256}, got ${actual}.`,
394+
)
327395
}
328396

329397
const fallbackName =
@@ -337,6 +405,8 @@ async function prepareInstallCandidate(
337405
...prepared,
338406
sourceDescription: entry.source,
339407
trust: typeof entry.trust === 'string' ? entry.trust : 'registry',
408+
toolsRequired: stringArray(entry.tools_required),
409+
minOpenClaudeVersion,
340410
}
341411
}
342412

@@ -373,6 +443,16 @@ export async function skillsInstallHandler(
373443
console.log(`Installing skill "${candidate.skillName}"`)
374444
console.log(`Source: ${candidate.sourceDescription}`)
375445
console.log(`Trust: ${candidate.trust}`)
446+
const trustWarning = trustInstallWarning(candidate.trust)
447+
if (trustWarning) {
448+
console.warn(trustWarning)
449+
}
450+
if (candidate.toolsRequired.length > 0) {
451+
console.log(`Tools required: ${candidate.toolsRequired.join(', ')}`)
452+
}
453+
if (candidate.minOpenClaudeVersion) {
454+
console.log(`Requires OpenClaude: >= ${candidate.minOpenClaudeVersion}`)
455+
}
376456
console.log(`Target: ${getDisplayPath(targetDir)}`)
377457

378458
await mkdir(root, { recursive: true })

src/skills/loadSkillsDir.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ test('loads flat and nested skills with colon namespaces', async () => {
111111
process.env.CLAUDE_CONFIG_DIR = originalConfigDir
112112
}
113113
clearSkillCaches()
114+
setAllowedSettingSources(originalSources)
114115
rmSync(configDir, { recursive: true, force: true })
115116
} finally {
116117
releaseSharedMutationLock()

src/utils/knowledgeGraph.stress.test.ts

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, it, beforeEach, afterEach, afterAll } from 'bun:test'
1+
import { describe, expect, it, beforeEach, afterEach } from 'bun:test'
22
import {
33
addGlobalEntity,
44
addGlobalSummary,
@@ -18,8 +18,7 @@ import { getFsImplementation } from './fsOperations.js'
1818
describe('KnowledgeGraph Phase 1 Stress & Edge Cases', () => {
1919
const originalConfigDir = process.env.CLAUDE_CONFIG_DIR
2020
const originalOrama = process.env.OPENCLAUDE_KNOWLEDGE_ORAMA
21-
const configDir = mkdtempSync(join(tmpdir(), 'openclaude-stress-'))
22-
const cwd = getFsImplementation().cwd()
21+
let configDir: string | undefined
2322

2423
const removeDirWithRetry = (dir: string) => {
2524
for (let attempt = 0; attempt < 5; attempt++) {
@@ -47,6 +46,7 @@ describe('KnowledgeGraph Phase 1 Stress & Edge Cases', () => {
4746

4847
beforeEach(async () => {
4948
await acquireEnvMutex()
49+
configDir = mkdtempSync(join(tmpdir(), 'openclaude-stress-'))
5050
process.env.CLAUDE_CONFIG_DIR = configDir
5151
process.env.OPENCLAUDE_KNOWLEDGE_ORAMA = '1'
5252
setClaudeConfigHomeDirForTesting(configDir)
@@ -69,14 +69,18 @@ describe('KnowledgeGraph Phase 1 Stress & Edge Cases', () => {
6969
}
7070
setClaudeConfigHomeDirForTesting(undefined)
7171
} finally {
72-
releaseEnvMutex()
72+
const dirToRemove = configDir
73+
configDir = undefined
74+
try {
75+
if (dirToRemove) {
76+
removeDirWithRetry(dirToRemove)
77+
}
78+
} finally {
79+
releaseEnvMutex()
80+
}
7381
}
7482
})
7583

76-
afterAll(() => {
77-
removeDirWithRetry(configDir)
78-
})
79-
8084
it('handles high-volume entity insertion (Stress Test)', async () => {
8185
const count = 50
8286

@@ -109,6 +113,7 @@ describe('KnowledgeGraph Phase 1 Stress & Edge Cases', () => {
109113
// 1. Create a valid DB
110114
await addGlobalEntity('type', 'valid', { val: '1' })
111115
const { getOramaPersistencePath } = await import('./knowledgeGraph.js')
116+
const cwd = getFsImplementation().cwd()
112117
const oramaPath = getOramaPersistencePath(cwd)
113118
expect(existsSync(oramaPath)).toBe(true)
114119

@@ -128,6 +133,8 @@ describe('KnowledgeGraph Phase 1 Stress & Edge Cases', () => {
128133
const { readdirSync } = await import('fs')
129134
// Search recursively from the actual Orama location. The config home can
130135
// be redirected by other tests, but the persisted file path is authoritative.
136+
const projectDir = dirname(oramaPath)
137+
expect(existsSync(projectDir)).toBe(true)
131138
const findCorrupted = (dir: string): boolean => {
132139
const entries = readdirSync(dir, { withFileTypes: true })
133140
for (const entry of entries) {
@@ -139,7 +146,7 @@ describe('KnowledgeGraph Phase 1 Stress & Edge Cases', () => {
139146
}
140147
return false
141148
}
142-
expect(findCorrupted(dirname(oramaPath))).toBe(true)
149+
expect(findCorrupted(projectDir)).toBe(true)
143150
})
144151

145152
it('maintains consistency between JSON and Orama', async () => {

0 commit comments

Comments
 (0)