Skip to content

Remove custom skills - #86

Merged
alvinunreal merged 2 commits into
masterfrom
remove-omos
Jan 25, 2026
Merged

Remove custom skills#86
alvinunreal merged 2 commits into
masterfrom
remove-omos

Conversation

@alvinunreal

Copy link
Copy Markdown
Owner

Migrating to pre-built skills

@greptile-apps

greptile-apps Bot commented Jan 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Overview

Greptile Summary

This PR migrates from a custom skill implementation to pre-built skills installed via npx skills add. The changes remove approximately 1,868 lines of custom skill infrastructure and replace it with a simpler system that delegates skill management to external packages.

Major changes:

  • Deleted custom skill implementation (src/tools/skill/builtin.ts, mcp-manager.ts, tools.ts, types.ts) totaling ~750 LOC
  • Added new src/cli/skills.ts to manage skill installation and permissions via external CLI
  • Extracted MCP configuration logic to src/config/agent-mcps.ts for better separation of concerns
  • Refactored src/cli/providers.ts to consolidate duplicated preset generation logic
  • Updated agent initialization to use the new skill permission system
  • Added skill installation flow to the CLI installer

Key improvements:

  • Simplified codebase by removing complex custom skill infrastructure
  • Better separation of concerns with config utilities moved to dedicated files
  • Eliminated code duplication in preset generation
  • Added backward compatibility support for legacy agent aliases

Issues found:

  • Command parsing bug in src/cli/skills.ts:75 that won't handle quoted arguments correctly

Confidence Score: 4/5

  • This PR is safe to merge with one minor fix needed
  • The refactoring is well-executed with proper test coverage and clear architectural improvements. The command parsing bug on line 75 is a logic issue that could cause failures with certain post-install commands containing spaces or quotes, but it only affects the agent-browser skill's post-install commands currently. The rest of the migration is clean and maintains backward compatibility.
  • Pay attention to src/cli/skills.ts - fix the command parsing logic before merge

Important Files Changed

Filename Overview
src/cli/skills.ts New file implementing skill installation and permissions. Contains command parsing issue on line 75.
src/config/agent-mcps.ts New file extracting MCP configuration logic from deleted builtin.ts. Clean refactoring with no issues.
src/agents/index.ts Updated to use new skill permissions system. Removed duplicate backward-compatibility code, now delegated to utils.
src/cli/providers.ts Simplified config generation by consolidating duplicated preset creation logic into reusable functions.
src/cli/install.ts Added skill installation flow to setup process. Clear implementation with proper error handling.
src/index.ts Removed custom skill tool registration and MCP manager. Import moved for parseList function.

Sequence Diagram

sequenceDiagram
    participant User
    participant Install as CLI Install
    participant Skills as skills.ts
    participant Providers as providers.ts
    participant AgentMCPs as agent-mcps.ts
    participant Agents as agents/index.ts
    participant Config as Config System

    User->>Install: bunx oh-my-opencode-slim install
    Install->>User: Ask configuration questions
    User->>Install: Provide answers (Antigravity, OpenAI, skills)
    
    alt Install Skills Enabled
        Install->>Skills: installSkill(skill)
        Skills->>Skills: npx skills add <repo>
        Skills->>Skills: Run postInstallCommands
        Skills-->>Install: Installation result
    end
    
    Install->>Providers: generateLiteConfig(config)
    Providers->>Skills: Get RECOMMENDED_SKILLS
    Skills-->>Providers: Return skill list
    Providers->>AgentMCPs: Get DEFAULT_AGENT_MCPS
    AgentMCPs-->>Providers: Return MCP defaults
    Providers->>Providers: createAgentConfig for each agent
    Providers->>Providers: buildPreset with skills & MCPs
    Providers-->>Install: Return config object
    
    Install->>Config: writeLiteConfig(config)
    Config-->>Install: Config written
    
    Note over User,Config: Runtime: Agent Initialization
    
    Config->>Agents: createAgents(config)
    Agents->>AgentMCPs: getAgentMcpList(agentName)
    AgentMCPs-->>Agents: Return MCP list
    Agents->>Skills: getSkillPermissionsForAgent(agentName)
    Skills-->>Agents: Return skill permissions
    Agents->>Agents: applyDefaultPermissions with skill rules
    Agents-->>Config: Return agent definitions

Loading

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment thread src/cli/skills.ts
Comment on lines +50 to +52
} catch (error) {
console.error(`Failed to install skill: ${skill.name}`);
return false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error details are lost when installation fails - only the skill name is logged. Consider logging the actual error for debugging.

Suggested change
} catch (error) {
console.error(`Failed to install skill: ${skill.name}`);
return false;
} catch (error) {
console.error(`Failed to install skill: ${skill.name}`, error);
return false;
}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/skills.ts
Line: 50:52

Comment:
Error details are lost when installation fails - only the skill name is logged. Consider logging the actual error for debugging.

```suggestion
    } catch (error) {
        console.error(`Failed to install skill: ${skill.name}`, error);
        return false;
    }
```

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

@alvinunreal
alvinunreal force-pushed the remove-omos branch 7 times, most recently from b215ebe to 303e235 Compare January 25, 2026 17:17
@alvinunreal

Copy link
Copy Markdown
Owner Author

@greptile

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment thread src/cli/skills.ts
console.log(`Running post-install commands for ${skill.name}...`);
for (const cmd of skill.postInstallCommands) {
console.log(`> ${cmd}`);
const [command, ...cmdArgs] = cmd.split(' ');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

splitting commands with split(' ') doesn't handle quoted arguments or arguments with spaces. Commands like npm install -g "my package" will break.

Suggested change
const [command, ...cmdArgs] = cmd.split(' ');
const [command, ...cmdArgs] = cmd.match(/(?:[^\s"]+|"[^"]*")+/g)?.map(arg => arg.replace(/^"|"$/g, '')) ?? [];
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/skills.ts
Line: 75:75

Comment:
splitting commands with `split(' ')` doesn't handle quoted arguments or arguments with spaces. Commands like `npm install -g "my package"` will break.

```suggestion
                const [command, ...cmdArgs] = cmd.match(/(?:[^\s"]+|"[^"]*")+/g)?.map(arg => arg.replace(/^"|"$/g, '')) ?? [];
```

How can I resolve this? If you propose a fix, please make it concise.

@alvinunreal
alvinunreal merged commit a95b94d into master Jan 25, 2026
2 checks passed
nghyane pushed a commit to nghyane/oh-my-opencode-slim that referenced this pull request Jan 31, 2026
* Remove custom skills

* Cleanups
@mhenke
mhenke deleted the remove-omos branch July 10, 2026 16:14
mhenke pushed a commit to mhenke/oh-my-opencode-slim that referenced this pull request Jul 17, 2026
* Remove custom skills

* Cleanups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant