Per agent MCPs - #78
Conversation
1301e2d to
a7451f7
Compare
Greptile SummaryThis PR implements per-agent MCP (Model Context Protocol) permissions, allowing fine-grained control over which agents can access which MCP servers. The implementation mirrors the existing skill permission system with wildcard ( Key Changes:
Implementation Quality: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Plugin as Plugin (index.ts)
participant AgentFactory as Agent Factory (agents/index.ts)
participant BuiltinSkills as Builtin Skills (tools/skill/builtin.ts)
participant SkillTools as Skill Tools (tools/skill/tools.ts)
participant OpenCodeSDK as OpenCode SDK
Note over User,OpenCodeSDK: Plugin Initialization
User->>Plugin: Initialize plugin with config
Plugin->>AgentFactory: getAgentConfigs(config)
AgentFactory->>BuiltinSkills: getAgentMcpList(agentName, config)
BuiltinSkills->>BuiltinSkills: Check config override or use DEFAULT_AGENT_MCPS
BuiltinSkills-->>AgentFactory: Return mcps list (e.g., ["websearch", "context7"])
AgentFactory-->>Plugin: Return agents with mcps field
Plugin->>Plugin: Compile MCP permissions
loop For each agent
Plugin->>BuiltinSkills: parseList(agentMcps, allMcpNames)
BuiltinSkills-->>Plugin: Return allowed MCPs
loop For each MCP
Plugin->>Plugin: Set permission rule (e.g., "websearch_*": "allow")
end
end
Plugin->>OpenCodeSDK: Register agents with permissions
Note over User,OpenCodeSDK: Skill Invocation with MCP
User->>SkillTools: Invoke omos_skill_mcp(skillName, mcpName, toolName)
SkillTools->>BuiltinSkills: canAgentUseMcp(agentName, mcpName, config)
BuiltinSkills->>BuiltinSkills: getAgentMcpList(agentName, config)
BuiltinSkills->>BuiltinSkills: parseList(mcpList, availableMcps)
BuiltinSkills-->>SkillTools: Return true/false
alt Agent has permission
SkillTools->>SkillTools: Call MCP tool
SkillTools-->>User: Return result
else Agent lacks permission
SkillTools-->>User: Throw error
end
|
| } from './tools'; | ||
| import { startTmuxCheck } from './utils'; | ||
| import { log } from './utils/logger'; | ||
| import { canAgentUseMcp, parseList } from './tools/skill/builtin'; |
There was a problem hiding this comment.
style: importing canAgentUseMcp but it's not used in this file
| import { canAgentUseMcp, parseList } from './tools/skill/builtin'; | |
| import { parseList } from './tools/skill/builtin'; |
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/index.ts
Line: 26:26
Comment:
**style:** importing `canAgentUseMcp` but it's not used in this file
```suggestion
import { parseList } from './tools/skill/builtin';
```
<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.| const permissionConfig = opencodeConfig.permission as | ||
| | Record<string, unknown> | ||
| | undefined; |
There was a problem hiding this comment.
style: permissionConfig is declared but never used in the permission compilation logic
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/index.ts
Line: 120:122
Comment:
**style:** `permissionConfig` is declared but never used in the permission compilation logic
<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.7a984d0 to
6b2a0f6
Compare
6b2a0f6 to
30f519d
Compare
| describe('parseList', () => { | ||
| test('returns empty array for empty input', () => { | ||
| expect(parseList([], ['a', 'b', 'c'])).toEqual([]); | ||
| }); | ||
|
|
||
| test('returns empty array for undefined input', () => { | ||
| expect(parseList(undefined as any, ['a', 'b', 'c'])).toEqual([]); | ||
| }); | ||
|
|
||
| test('returns explicit items when no wildcard', () => { | ||
| expect(parseList(['a', 'c'], ['a', 'b', 'c'])).toEqual(['a', 'c']); | ||
| }); | ||
|
|
||
| test('expands wildcard to all available items', () => { | ||
| expect(parseList(['*'], ['a', 'b', 'c'])).toEqual(['a', 'b', 'c']); | ||
| }); | ||
|
|
||
| test('excludes items with ! prefix', () => { | ||
| expect(parseList(['*', '!b'], ['a', 'b', 'c'])).toEqual(['a', 'c']); | ||
| }); | ||
|
|
||
| test('excludes multiple items with ! prefix', () => { | ||
| expect(parseList(['*', '!b', '!c'], ['a', 'b', 'c', 'd'])).toEqual([ | ||
| 'a', | ||
| 'd', | ||
| ]); | ||
| }); | ||
|
|
||
| test('deny wins in case of conflict', () => { | ||
| expect(parseList(['a', '!a'], ['a', 'b'])).toEqual([]); | ||
| }); | ||
|
|
||
| test('!* denies all items', () => { | ||
| expect(parseList(['!*'], ['a', 'b', 'c'])).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('DEFAULT_AGENT_MCPS', () => { | ||
| test('orchestrator has websearch MCP', () => { | ||
| expect(DEFAULT_AGENT_MCPS.orchestrator).toContain('websearch'); | ||
| }); | ||
|
|
||
| test('designer has no MCPs by default', () => { | ||
| expect(DEFAULT_AGENT_MCPS.designer).toEqual([]); | ||
| }); | ||
|
|
||
| test('oracle has no MCPs by default', () => { | ||
| expect(DEFAULT_AGENT_MCPS.oracle).toEqual([]); | ||
| }); | ||
|
|
||
| test('librarian has websearch, context7, and grep_app MCPs', () => { | ||
| expect(DEFAULT_AGENT_MCPS.librarian).toContain('websearch'); | ||
| expect(DEFAULT_AGENT_MCPS.librarian).toContain('context7'); | ||
| expect(DEFAULT_AGENT_MCPS.librarian).toContain('grep_app'); | ||
| }); | ||
|
|
||
| test('explorer has no MCPs by default', () => { | ||
| expect(DEFAULT_AGENT_MCPS.explorer).toEqual([]); | ||
| }); | ||
|
|
||
| test('fixer has no MCPs by default', () => { | ||
| expect(DEFAULT_AGENT_MCPS.fixer).toEqual([]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
logic: Duplicate test suites - parseList and DEFAULT_AGENT_MCPS are tested twice with identical test cases
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/skill/builtin.test.ts
Line: 438:501
Comment:
**logic:** Duplicate test suites - parseList and DEFAULT_AGENT_MCPS are tested twice with identical test cases
How can I resolve this? If you propose a fix, please make it concise.| describe('getAgentMcpList', () => { | ||
| test('returns default MCPs for orchestrator', () => { | ||
| const mcps = getAgentMcpList('orchestrator'); | ||
| expect(mcps).toEqual(DEFAULT_AGENT_MCPS.orchestrator); | ||
| }); | ||
|
|
||
| test('returns default MCPs for librarian', () => { | ||
| const mcps = getAgentMcpList('librarian'); | ||
| expect(mcps).toEqual(DEFAULT_AGENT_MCPS.librarian); | ||
| }); | ||
|
|
||
| test('returns empty for designer', () => { | ||
| const mcps = getAgentMcpList('designer'); | ||
| expect(mcps).toEqual([]); | ||
| }); | ||
|
|
||
| test('respects config override for agent MCPs', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| oracle: { mcps: ['websearch'] }, | ||
| }, | ||
| }; | ||
| const mcps = getAgentMcpList('oracle', config); | ||
| expect(mcps).toEqual(['websearch']); | ||
| }); | ||
|
|
||
| test('config wildcard overrides default', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| designer: { mcps: ['*'] }, | ||
| }, | ||
| }; | ||
| const mcps = getAgentMcpList('designer', config); | ||
| expect(mcps).toEqual(['*']); | ||
| }); | ||
|
|
||
| test('config empty array removes default MCPs', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| librarian: { mcps: [] }, | ||
| }, | ||
| }; | ||
| const mcps = getAgentMcpList('librarian', config); | ||
| expect(mcps).toEqual([]); | ||
| }); | ||
|
|
||
| test('backward compat: alias config applies to agent', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| explore: { mcps: ['websearch'] }, | ||
| }, | ||
| }; | ||
| const mcps = getAgentMcpList('explorer', config); | ||
| expect(mcps).toEqual(['websearch']); | ||
| }); | ||
|
|
||
| test('returns empty for unknown agent without config', () => { | ||
| const mcps = getAgentMcpList('unknown-agent'); | ||
| expect(mcps).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('canAgentUseMcp', () => { | ||
| test('orchestrator can use websearch by default', () => { | ||
| expect(canAgentUseMcp('orchestrator', 'websearch')).toBe(true); | ||
| }); | ||
|
|
||
| test('librarian can use websearch, context7, and grep_app by default', () => { | ||
| expect(canAgentUseMcp('librarian', 'websearch')).toBe(true); | ||
| expect(canAgentUseMcp('librarian', 'context7')).toBe(true); | ||
| expect(canAgentUseMcp('librarian', 'grep_app')).toBe(true); | ||
| }); | ||
|
|
||
| test('designer cannot use any MCP by default', () => { | ||
| expect(canAgentUseMcp('designer', 'websearch')).toBe(false); | ||
| expect(canAgentUseMcp('designer', 'context7')).toBe(false); | ||
| }); | ||
|
|
||
| test('respects config override', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| oracle: { mcps: ['websearch'] }, | ||
| }, | ||
| }; | ||
| expect(canAgentUseMcp('oracle', 'websearch', config)).toBe(true); | ||
| expect(canAgentUseMcp('oracle', 'context7', config)).toBe(false); | ||
| }); | ||
|
|
||
| test('config wildcard grants all MCP permissions', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| designer: { mcps: ['*'] }, | ||
| }, | ||
| }; | ||
| expect(canAgentUseMcp('designer', 'websearch', config)).toBe(true); | ||
| }); | ||
|
|
||
| test('config wildcard grants skill MCP permissions', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| designer: { mcps: ['*'] }, | ||
| }, | ||
| }; | ||
| expect(canAgentUseMcp('designer', 'playwright', config)).toBe(true); | ||
| }); | ||
|
|
||
| test('config empty array denies all MCPs', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| librarian: { mcps: [] }, | ||
| }, | ||
| }; | ||
| expect(canAgentUseMcp('librarian', 'websearch', config)).toBe(false); | ||
| }); | ||
|
|
||
| test('respects exclusion syntax', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| orchestrator: { mcps: ['*', '!websearch'] }, | ||
| }, | ||
| }; | ||
| // canAgentUseMcp uses DEFAULT_AGENT_MCPS.orchestrator keys as allAvailable | ||
| // which is ['websearch'], so excluding websearch leaves empty | ||
| expect(canAgentUseMcp('orchestrator', 'websearch', config)).toBe(false); | ||
| }); | ||
|
|
||
| test('backward compat: alias config affects agent permissions', () => { | ||
| const config: PluginConfig = { | ||
| agents: { | ||
| explore: { mcps: ['websearch'] }, | ||
| }, | ||
| }; | ||
| expect(canAgentUseMcp('explorer', 'websearch', config)).toBe(true); | ||
| expect(canAgentUseMcp('explorer', 'context7', config)).toBe(false); | ||
| }); | ||
|
|
||
| test('unknown agent returns false without config', () => { | ||
| expect(canAgentUseMcp('unknown-agent', 'websearch')).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
logic: Duplicate test suites - getAgentMcpList and canAgentUseMcp test suites appear to be duplicated
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/skill/builtin.test.ts
Line: 503:642
Comment:
**logic:** Duplicate test suites - getAgentMcpList and canAgentUseMcp test suites appear to be duplicated
How can I resolve this? If you propose a fix, please make it concise.30f519d to
97ac2df
Compare
| const permissionConfig = opencodeConfig.permission as | ||
| | Record<string, unknown> | ||
| | undefined; |
There was a problem hiding this comment.
style: permissionConfig is declared but never used
| const permissionConfig = opencodeConfig.permission as | |
| | Record<string, unknown> | |
| | undefined; | |
| // Compile MCP permissions into PermissionNext rules | |
| // This maps our simple mcps config to opencode's permission system |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/index.ts
Line: 120:122
Comment:
**style:** `permissionConfig` is declared but never used
```suggestion
// Compile MCP permissions into PermissionNext rules
// This maps our simple mcps config to opencode's permission system
```
How can I resolve this? If you propose a fix, please make it concise.| } from './tools'; | ||
| import { startTmuxCheck } from './utils'; | ||
| import { log } from './utils/logger'; | ||
| import { canAgentUseMcp, parseList } from './tools/skill/builtin'; |
There was a problem hiding this comment.
style: canAgentUseMcp is imported but never used in this file
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/index.ts
Line: 26:26
Comment:
**style:** `canAgentUseMcp` is imported but never used in this file
How can I resolve this? If you propose a fix, please make it concise.3bf888c to
7eae8c6
Compare
7eae8c6 to
78815e3
Compare
Summary
Changes