Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions src/lib/config-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
type OpencodeClientLike,
} from './model-availability.js'
import {
formatSkillDescription,
type LoadedSkill,
loadSkill,
wrapSkillTemplate,
Expand Down Expand Up @@ -57,9 +56,7 @@ function isSystematicCommandConfig(
): boolean {
const description = command?.description
return (
typeof description === 'string' &&
(description.startsWith('(Systematic) ') ||
description.startsWith('(Systematic - Skill) '))
typeof description === 'string' && description.startsWith('(Systematic) ')
)
}

Expand Down Expand Up @@ -515,7 +512,7 @@ function collectSkillsAsCommands(
* OpenCode's skill registry or permissions here to compensate.
*/
function loadDiscoveredSkillAsCommand(skill: DiscoveredSkill): CommandConfig {
const description = formatSkillDescription(skill.description, skill.name)
const description = skill.description || `${skill.name} skill`

if (skill.frontmatter.disableModelInvocation === true) {
// Body was read once at discovery time (DiscoveredSkill.body); no re-read.
Expand Down Expand Up @@ -728,15 +725,13 @@ export function createConfigHandler(deps: ConfigHandlerDeps) {
...bundledSkills,
...discoveredSkillCommands,
}
// Drop any existing entry Systematic previously emitted (identified by
// the `(Systematic) `/`(Systematic - Skill) ` description marker),
// regardless of key. If it's still valid this pass, `emittedCommands`
// re-adds it below (mergeSystematicEntries drops-then-adds); if the
// source (bundled command/skill, or a discovered on-disk skill) no
// longer exists, it stays dropped. This also fixes orphaned
// discovered-skill commands: those use a bare key (not a
// `systematic:`/`ce:`-prefixed key), so removing a skill from disk left
// its stale command behind under the old, narrower key-based predicate.
// Bundled commands/skills carry the `(Systematic) ` marker and are
// dropped-then-re-emitted every load (refresh semantics). Discovered-skill
// commands are intentionally UNMARKED — they're the user's own skills,
// not Systematic's — so they're add-if-absent and never refreshed or
// cleaned up across reloads. That's fine: OpenCode rebuilds config
// in-memory on every launch, so a deleted skill just isn't re-emitted;
// nothing stale persists to disk.
config.command = mergeSystematicEntries(
existingCommands as Record<string, CommandConfig>,
emittedCommands as Record<string, CommandConfig>,
Expand Down
2 changes: 1 addition & 1 deletion src/lib/skill-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { parseFrontmatter } from './frontmatter.js'
import type { SkillDeprecated, SkillInfo } from './skills.js'

const SKILL_PREFIX = 'systematic:'
const SKILL_DESCRIPTION_PREFIX = '(Systematic - Skill) '
const SKILL_DESCRIPTION_PREFIX = '(Systematic) '

export interface LoadedSkill {
name: string
Expand Down
63 changes: 39 additions & 24 deletions tests/unit/config-handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ Command template for ${name}.`,
expect(config.command).toBeDefined()
expect(config.command?.['systematic:test-skill']).toBeDefined()
expect(config.command?.['systematic:test-skill']?.description).toBe(
'(Systematic - Skill) A test skill',
'(Systematic) A test skill',
)
expect(config.command?.['systematic:test-skill']?.template).toContain(
'<skill-instruction>',
Expand Down Expand Up @@ -295,7 +295,7 @@ Skill content for ce:plan.`,
expect(config.command?.['ce:plan']).toBeDefined()
expect(config.command?.['systematic:ce:plan']).toBeUndefined()
expect(config.command?.['ce:plan']?.description).toBe(
'(Systematic - Skill) Plan workflow skill',
'(Systematic) Plan workflow skill',
)
})

Expand Down Expand Up @@ -420,7 +420,7 @@ Skill content for ce:plan.`,
template: 'native command template',
})
expect(config.command?.['systematic:test-skill']?.description).toBe(
'(Systematic - Skill) A test skill',
'(Systematic) A test skill',
)
})

Expand Down Expand Up @@ -483,23 +483,23 @@ Skill content for ce:plan.`,
template: 'native command template',
},
'ce:plan': {
description: '(Systematic - Skill) Old plan command',
description: '(Systematic) Old plan command',
template: 'old plan template',
},
'systematic:legacy-command': {
description: '(Systematic) Old command',
template: 'old command template',
},
'systematic:legacy-skill': {
description: '(Systematic - Skill) Old skill command',
description: '(Systematic) Old skill command',
template: 'old skill template',
},
'systematic:test-command': {
description: '(Systematic) Old command',
template: 'old command template',
},
'systematic:test-skill': {
description: '(Systematic - Skill) Old skill command',
description: '(Systematic) Old skill command',
template: 'old skill template',
},
},
Expand All @@ -518,17 +518,14 @@ Skill content for ce:plan.`,
'Agent prompt for systematic-implementer.',
)
// Unlike agents (which still use a key-based `bundledAgentKeys` guard),
// the command drop predicate was simplified to drop ANY existing entry
// carrying the `(Systematic) `/`(Systematic - Skill) ` marker,
// regardless of key — this is required to clean up orphaned
// discovered-skill commands (which use bare, unprefixed keys with no
// way to distinguish "still owned" from "stale" other than the
// marker). A side effect: a bare, non-prefixed key that merely *looks*
// like Systematic output but isn't re-emitted (like this synthetic
// 'native-command' fixture) is now dropped rather than preserved.
// the command drop predicate drops ANY existing entry carrying the
// `(Systematic) ` marker, regardless of key. A side effect: a bare,
// non-prefixed key that merely *looks* like Systematic output but isn't
// re-emitted (like this synthetic 'native-command' fixture) is now
// dropped rather than preserved.
expect(config.command?.['native-command']).toBeUndefined()
expect(config.command?.['ce:plan']?.description).toBe(
'(Systematic - Skill) A plan skill',
'(Systematic) A plan skill',
)
expect(config.command?.['ce:plan']?.template).toContain(
'Skill content for ce:plan',
Expand All @@ -542,7 +539,7 @@ Skill content for ce:plan.`,
'Command template for test-command',
)
expect(config.command?.['systematic:test-skill']?.description).toBe(
'(Systematic - Skill) A test skill',
'(Systematic) A test skill',
)
})

Expand Down Expand Up @@ -1750,9 +1747,7 @@ Discovered body for ${name}.`,

const command = config.command?.foo
expect(command).toBeDefined()
expect(command?.description).toBe(
'(Systematic - Skill) A discovered skill',
)
expect(command?.description).toBe('A discovered skill')
expect(command?.template).toContain('skill tool')
expect(command?.template).toContain('<user-request>\n$ARGUMENTS')
})
Expand All @@ -1775,9 +1770,7 @@ Discovered body for ${name}.`,

const command = config.command?.bar
expect(command).toBeDefined()
expect(command?.description).toBe(
'(Systematic - Skill) A command-only skill',
)
expect(command?.description).toBe('A command-only skill')
expect(command?.template).toContain('<skill-instruction>')
expect(command?.template).toContain('Discovered body for bar')
expect(command?.template).not.toContain('skill tool')
Expand Down Expand Up @@ -1851,7 +1844,7 @@ Discovered body for ${name}.`,
expect(Object.keys(config.command ?? {})).toEqual(['foo'])
})

test('orphaned discovered-skill command is removed once its skill is deleted from disk', async () => {
test('discovered-skill command persists after its skill is deleted from disk (unmarked, not refreshed)', async () => {
const discoveredDir = path.join(projectDir, '.opencode/skills')
writeDiscoveredSkill(discoveredDir, 'foo')

Expand All @@ -1871,8 +1864,11 @@ Discovered body for ${name}.`,
force: true,
})

// Discovered-skill commands carry no `(Systematic) ` marker, so the
// drop-then-re-emit refresh never touches them: this is intentional,
// since OpenCode rebuilds config in-memory on every launch anyway.
await handler(config)
expect(config.command?.foo).toBeUndefined()
expect(config.command?.foo).toBeDefined()
})

test('user-defined command of the same name still survives after the skill is removed (R4)', async () => {
Expand Down Expand Up @@ -1907,6 +1903,25 @@ Discovered body for ${name}.`,
expect(config.command?.foo?.template).toBe('User template')
})

test('discovered skill command description has no Systematic prefix (honest attribution)', async () => {
writeDiscoveredSkill(path.join(projectDir, '.opencode/skills'), 'baz', {
description: 'My own custom skill',
})

const handler = createConfigHandler({
directory: projectDir,
bundledSkillsDir: path.join(bundledDir, 'skills'),
bundledAgentsDir: path.join(bundledDir, 'agents'),
bundledCommandsDir: path.join(bundledDir, 'commands'),
})

const config: Config = {}
await handler(config)

expect(config.command?.baz?.description).toBe('My own custom skill')
expect(config.command?.baz?.description).not.toContain('(Systematic')
})

test('discovered skill with user-invocable: false does not emit a command', async () => {
const skillDir = path.join(projectDir, '.opencode/skills', 'hidden')
fs.mkdirSync(skillDir, { recursive: true })
Expand Down
14 changes: 7 additions & 7 deletions tests/unit/skill-loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,21 @@ describe('skill-loader', () => {
})

describe('formatSkillDescription', () => {
test('adds (Systematic - Skill) prefix to description', () => {
test('adds (Systematic) prefix to description', () => {
expect(formatSkillDescription('A test skill', 'test')).toBe(
'(Systematic - Skill) A test skill',
'(Systematic) A test skill',
)
})

test('does not double-prefix already prefixed description', () => {
expect(
formatSkillDescription('(Systematic - Skill) A test skill', 'test'),
).toBe('(Systematic - Skill) A test skill')
expect(formatSkillDescription('(Systematic) A test skill', 'test')).toBe(
'(Systematic) A test skill',
)
})

test('uses fallback name when description is empty', () => {
expect(formatSkillDescription('', 'my-skill')).toBe(
'(Systematic - Skill) my-skill skill',
'(Systematic) my-skill skill',
)
})
})
Expand Down Expand Up @@ -165,7 +165,7 @@ description: A test skill

expect(loaded.name).toBe('test-skill')
expect(loaded.prefixedName).toBe('systematic:test-skill')
expect(loaded.description).toBe('(Systematic - Skill) A test skill')
expect(loaded.description).toBe('(Systematic) A test skill')
expect(loaded.wrappedTemplate).toContain('<skill-instruction>')
expect(loaded.wrappedTemplate).toContain('# Test Content')
})
Expand Down
Loading