fix(skills): resolve code review issues — migration guard, config safety, type cleanup#280
Conversation
mykolanehrych
force-pushed
the
EPMCDME-11906_fix_setup_skills_profile
branch
from
d2ac7a8 to
082b3e9
Compare
- Add `codemie setup skills` CLI command for interactive skill registration - Migrate codemieSkills/codemieAssistants to top-level config (migration 004) - Add skill slug format migration with project/scope suffix (migration 005) - Add unique slug generation in claude-skill-generator - Extract shared selection UI and helpers from assistants/skills setup - Fix assistant summary display and setup API alignment - Add auto-sync on agent startup with setup disclaimer Generated with AI Co-Authored-By: codemie-ai <codemie.ai@gmail.com>
mykolanehrych
force-pushed
the
EPMCDME-11906_fix_setup_skills_profile
branch
from
082b3e9 to
df066c9
Compare
mykolanehrych
changed the title
…up_skills_profile
| await ConfigLoader.saveProfile(profileName, config); | ||
| configLocation = `global (~/.codemie/codemie-cli.config.json)`; | ||
| } | ||
| const configLocation = storageScope === 'local' |
There was a problem hiding this comment.
Looks like this is not the right place for getting the config location.
I suggest it to be moved somewhere to config class so it's reusable.
Also I suggest the storage scopes to be an emum
| */ | ||
| function generateName(skill: SkillDetail): string { | ||
| const baseName = skill.name.toLowerCase() | ||
| .replace(/[^a-z0-9]+/g, '-') |
There was a problem hiding this comment.
I suggest we move teh regexps to constants.
That way we can understand what they do by the constant name
| const base = baseName || skill.id.toLowerCase().replace(/[^a-z0-9]+/g, '-'); | ||
|
|
||
| const projectSuffix = skill.project | ||
| ? `-${skill.project.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '')}` |
There was a problem hiding this comment.
Or event better - let's move them to separate functions, e.x. sanitazeSkillName or smth
| await ConfigLoader.saveSkillsToProjectConfig(workingDir, storageScope, updatedSkills); | ||
|
|
||
| const configLocation = storageScope === 'local' | ||
| ? `${workingDir}/.codemie/codemie-cli.config.json` |
There was a problem hiding this comment.
Duplicate code for fetching the config location
| ); | ||
| if (hasProfileLevelSkills && !globalConfig.codemieSkills?.length) { | ||
| throw new Error( | ||
| '005-skill-slug-format migration requires migration 004 (move-skills-to-top-level) to have run first. ' + |
There was a problem hiding this comment.
Is there a way we can save the applied migrations so we don't have to do the manual checks? Smth like alembic so we store the last migration id or smth.
Also I suggest investigating the already existing libaries, maybe there is some that can fit here.
https://github.com/sequelize/umzug that I found bit haven't used so not sure
| if (config.profiles[profileName]) { | ||
| config.profiles[profileName].codemieSkills = skills; | ||
| if (scope === 'global') { | ||
| const config = await this.loadMultiProviderConfig(); |
There was a problem hiding this comment.
How about we do the loadProviderConfig(scope) function and then let it take the globa/local logic, maybe use subfunctions? So we don't have to do if scope === else ... every time
2 participants
EPMCDME-11906, EPMCDME-11925
Summary
Spec
skills-assistants-top-level.md
QA Guide
qa-migration-skills-assistants.md