t008.2: MCP registration for OpenCode plugin#1149
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe PR introduces comprehensive MCP (Model Context Protocol) server registration and management to the OpenCode AIDevOps plugin, adding platform detection, package runner resolution, a data-driven registry, and runtime enablement of MCPs with per-agent tool mappings and global disable patterns. Changes
Sequence DiagramsequenceDiagram
participant ConfigHook as Config Hook
participant Registry as MCP Registry
participant PkgMgr as Package Resolver
participant RegFunc as registerMcpServers()
participant ToolMapper as applyAgentMcpTools()
participant Config as Config Object
ConfigHook->>Registry: Phase 1: Load subagents from frontmatter
ConfigHook->>Registry: Retrieve MCP registry metadata
Registry-->>ConfigHook: Return registry with platform/dependency info
ConfigHook->>PkgMgr: Detect platform (IS_MACOS)
ConfigHook->>PkgMgr: Resolve package runner (bun x vs npx)
PkgMgr-->>ConfigHook: Return runner preference
ConfigHook->>RegFunc: Phase 2: Call registerMcpServers(config)
loop For each MCP in registry
RegFunc->>RegFunc: Check platform constraints (skip macOS-only on non-macOS)
RegFunc->>RegFunc: Verify required binaries present
RegFunc->>RegFunc: Register MCP (local or remote) with eager/lazy setting
RegFunc->>Config: Apply tool permissions per MCP toolPattern
end
RegFunc->>Config: Disable OMO_DISABLED_PATTERNS globally
RegFunc-->>ConfigHook: Return count of registered MCPs
ConfigHook->>ToolMapper: Call applyAgentMcpTools(config)
loop For each agent in AGENT_MCP_TOOLS
ToolMapper->>Config: Enable specific MCP tool patterns per agent
end
ToolMapper-->>ConfigHook: Return
ConfigHook->>Config: Log summary: agents, MCPs, tool mappings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the aidevops OpenCode plugin by integrating a robust system for dynamic MCP (Micro-Capability Platform) server registration and management. It introduces a data-driven registry for various tools, automates their setup based on platform and binary availability, and refines how tool permissions are assigned to individual agents. This change aims to improve the plugin's flexibility, reduce manual configuration, and ensure that agents have access to the necessary tools efficiently. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust, data-driven MCP registration system to the OpenCode plugin, enhancing MCP configuration. However, it contains several instances of insecure command construction and potential shell injection due to improper handling of paths, specifically with pkgRunner being split by spaces and interpolated without proper quoting. My review also highlights the need to improve security, maintainability, and consistency in the new JavaScript code, addressing bash -c usage that violates security style guides, inconsistent package runner usage, and other potential command injection vectors. The suggested changes aim to align the new code with best practices and the established style guide.
| command: [ | ||
| "/bin/bash", | ||
| "-c", | ||
| "OUTSCRAPER_API_KEY=$OUTSCRAPER_API_KEY uv tool run outscraper-mcp-server", | ||
| ], |
There was a problem hiding this comment.
This command uses bash -c, which is eval-like and violates the repository style guide's security rule against such constructs. Additionally, the OUTSCRAPER_API_KEY=$OUTSCRAPER_API_KEY part is confusing. If OUTSCRAPER_API_KEY is expected to be an exported environment variable, this part is redundant. If it's not, this will likely assign an empty string to the variable for the uv command, which would be a bug.
Assuming OUTSCRAPER_API_KEY is inherited from the environment, the bash -c wrapper is unnecessary and the command can be simplified.
command: ["uv", "tool", "run", "outscraper-mcp-server"],References
- The use of
bash -c "..."is a form ofeval, which is discouraged for security and maintainability. The style guide recommends using bash arrays for dynamic command construction where possible. (link)
| command: [ | ||
| "/bin/bash", | ||
| "-c", | ||
| `source ~/.config/aidevops/credentials.sh && DATAFORSEO_USERNAME=$DATAFORSEO_USERNAME DATAFORSEO_PASSWORD=$DATAFORSEO_PASSWORD ${pkgRunner} dataforseo-mcp-server`, |
There was a problem hiding this comment.
This command construction using bash -c is eval-like and violates the repository's security style guide. Interpolating pkgRunner directly into a shell command string is vulnerable to shell injection if the path contains spaces or shell metacharacters, as the path is not properly quoted. This complex, multi-part command string is also difficult to read, maintain, and debug. To improve security and maintainability, this logic should be encapsulated in a dedicated wrapper shell script, ensuring proper quoting for pkgRunner.
`source ~/.config/aidevops/credentials.sh && DATAFORSEO_USERNAME=$DATAFORSEO_USERNAME DATAFORSEO_PASSWORD=$DATAFORSEO_PASSWORD "${pkgRunnerParts.join(" "')}" dataforseo-mcp-server`References
- Shell command construction using
bash -ciseval-like and violates the repository style guide's security rule against it. (link)
| function getPkgRunner() { | ||
| if (_pkgRunner !== null) return _pkgRunner; | ||
| const bunPath = run("which bun"); | ||
| const npxPath = run("which npx"); | ||
| _pkgRunner = bunPath ? `${bunPath} x` : npxPath || "npx"; | ||
| return _pkgRunner; | ||
| } |
There was a problem hiding this comment.
The pkgRunner string, which contains the path to bun or npx, is constructed by concatenating the output of run("which ...") with a command (e.g., x). This string is then used in two insecure ways:
- Argument Splitting (Line 239): It is split by spaces:
const pkgRunnerParts = pkgRunner.split(" ");. If the path to the binary (e.g.,bunPath) contains spaces, the resulting array will be incorrect, leading to the execution of an unintended binary or failed execution. - Shell Interpolation (Line 302): It is interpolated directly into a shell command string passed to
/bin/bash -c. IfpkgRunnercontains shell metacharacters (like;,&, or spaces), it can lead to shell injection or incorrect command execution.
While the path is obtained via the which command, an attacker who can influence the environment (e.g., the PATH variable) or a user with a non-standard installation path containing spaces can trigger this behavior. In the case of shell interpolation, this could allow for arbitrary command execution in the context of the plugin.
let _pkgRunnerParts = null;
function getPkgRunnerParts() {
if (_pkgRunnerParts !== null) return _pkgRunnerParts;
const bunPath = run("which bun");
const npxPath = run("which npx");
if (bunPath) {
_pkgRunnerParts = [bunPath, "x"];
} else {
_pkgRunnerParts = [npxPath || "npx"];
}
return _pkgRunnerParts;
}| const pkgRunner = getPkgRunner(); | ||
| const pkgRunnerParts = pkgRunner.split(" "); |
There was a problem hiding this comment.
The use of pkgRunner.split(" ") is fragile and insecure if the path to the package runner contains spaces. It is better to use an array of command parts directly from the source.
const pkgRunnerParts = getPkgRunnerParts();| { | ||
| name: "shadcn", | ||
| type: "local", | ||
| command: ["npx", "shadcn@latest", "mcp"], |
There was a problem hiding this comment.
This command hardcodes npx, but the getMcpRegistry function determines a preferred package runner (pkgRunner) at the top. This is inconsistent with other commands like playwriter that correctly use the dynamic pkgRunner.
This should be updated for consistency and to respect the user's environment (e.g., prefer bun if available). This also applies to claude-code-mcp, macos-automator, and ios-simulator which also hardcode npx.
command: [...pkgRunnerParts, "shadcn@latest", "mcp"],| if (mcp.requiresBinary) { | ||
| const binaryPath = run(`which ${mcp.requiresBinary}`); | ||
| if (!binaryPath) { | ||
| // Disable tools if binary not available | ||
| if (mcp.toolPattern) { | ||
| config.tools[mcp.toolPattern] = false; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The mcp.requiresBinary value is directly interpolated into a command string for run() (which uses execSync). While the values are currently hardcoded and safe, this is a dangerous pattern that could lead to command injection if the MCP registry data were ever sourced from a less trusted location. It's a good practice to validate inputs before using them in shell commands.
if (mcp.requiresBinary) {
if (!/^[a-zA-Z0-9._-]+$/.test(mcp.requiresBinary)) {
console.error(`[aidevops] Invalid characters in requiresBinary for MCP '${mcp.name}': ${mcp.requiresBinary}`);
continue;
}
const binaryPath = run(`which ${mcp.requiresBinary}`);
if (!binaryPath) {
// Disable tools if binary not available
if (mcp.toolPattern) {
config.tools[mcp.toolPattern] = false;
}
continue;
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TODO.md (1)
643-647:⚠️ Potential issue | 🔴 CriticalResolve merge conflict markers before merge.
Unresolved conflict markers will break TODO parsing and downstream tooling.
🩹 Proposed fix
-<<<<<<< Updated upstream - - [x] t012.2 Design extraction schema (vendor, amount, date, VAT, items) ~4h `#auto-dispatch` blocked-by:t012.1 ref:GH#1116 completed:2026-02-11 -======= - - [x] t012.2 Design extraction schema (vendor, amount, date, VAT, items) ~4h `#auto-dispatch` blocked-by:t012.1 completed:2026-02-11 ->>>>>>> Stashed changes + - [x] t012.2 Design extraction schema (vendor, amount, date, VAT, items) ~4h `#auto-dispatch` blocked-by:t012.1 ref:GH#1116 completed:2026-02-11
🤖 Fix all issues with AI agents
In @.agents/plugins/opencode-aidevops/index.mjs:
- Around line 459-463: The loop that iterates OMO_DISABLED_PATTERNS currently
only sets config.tools[pattern] = false when the key is missing, but we must
enforce disabling unconditionally; update the loop to always assign false for
each pattern (i.e., remove the if (!(pattern in config.tools)) guard) and ensure
config.tools is initialized as an object before the loop so assignments won't
throw; reference OMO_DISABLED_PATTERNS and config.tools to locate and change the
code in the same block.
- Around line 420-430: When a local MCP declares requiresBinary but the binary
isn't found (the run(`which ${mcp.requiresBinary}`) branch), explicitly disable
any existing registration instead of merely continuing: set
config.tools[mcp.toolPattern] = false (as already done) and also clear or
disable the MCP entry in the app config/registry where MCPs are stored (e.g.,
the map/list keyed by mcp.name or mcp.id) so a previously-registered MCP cannot
stay enabled; keep the continue to skip registration after disabling. Ensure you
update the same config object used for runtime checks so the MCP cannot be used
when its binary is missing.
| // Skip local MCPs whose binary isn't installed | ||
| if (mcp.requiresBinary) { | ||
| const binaryPath = run(`which ${mcp.requiresBinary}`); | ||
| if (!binaryPath) { | ||
| // Disable tools if binary not available | ||
| if (mcp.toolPattern) { | ||
| config.tools[mcp.toolPattern] = false; | ||
| } | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Disable already-registered MCPs when required binaries are missing.
Right now you skip registration, but if the MCP already exists in config it can still be enabled and fail at runtime. This undermines the “skip if binary missing” behavior.
🛠️ Proposed fix
if (mcp.requiresBinary) {
const binaryPath = run(`which ${mcp.requiresBinary}`);
if (!binaryPath) {
// Disable tools if binary not available
if (mcp.toolPattern) {
config.tools[mcp.toolPattern] = false;
}
+ // Ensure previously-registered MCPs are disabled too
+ if (config.mcp[mcp.name]) {
+ config.mcp[mcp.name].enabled = false;
+ }
continue;
}
}🤖 Prompt for AI Agents
In @.agents/plugins/opencode-aidevops/index.mjs around lines 420 - 430, When a
local MCP declares requiresBinary but the binary isn't found (the run(`which
${mcp.requiresBinary}`) branch), explicitly disable any existing registration
instead of merely continuing: set config.tools[mcp.toolPattern] = false (as
already done) and also clear or disable the MCP entry in the app config/registry
where MCPs are stored (e.g., the map/list keyed by mcp.name or mcp.id) so a
previously-registered MCP cannot stay enabled; keep the continue to skip
registration after disabling. Ensure you update the same config object used for
runtime checks so the MCP cannot be used when its binary is missing.
| // Disable Oh-My-OpenCode tool patterns globally | ||
| for (const pattern of OMO_DISABLED_PATTERNS) { | ||
| if (!(pattern in config.tools)) { | ||
| config.tools[pattern] = false; | ||
| } |
There was a problem hiding this comment.
Enforce the global Oh‑My‑OpenCode disable list unconditionally.
The current guard only disables patterns if they were unset, which contradicts the “disable globally” intent.
🛠️ Proposed fix
// Disable Oh-My-OpenCode tool patterns globally
for (const pattern of OMO_DISABLED_PATTERNS) {
- if (!(pattern in config.tools)) {
- config.tools[pattern] = false;
- }
+ config.tools[pattern] = false;
}🤖 Prompt for AI Agents
In @.agents/plugins/opencode-aidevops/index.mjs around lines 459 - 463, The loop
that iterates OMO_DISABLED_PATTERNS currently only sets config.tools[pattern] =
false when the key is missing, but we must enforce disabling unconditionally;
update the loop to always assign false for each pattern (i.e., remove the if
(!(pattern in config.tools)) guard) and ensure config.tools is initialized as an
object before the loop so assignments won't throw; reference
OMO_DISABLED_PATTERNS and config.tools to locate and change the code in the same
block.
Adds data-driven MCP registry with 12 server definitions (osgrep, playwriter, context7, augment-context-engine, sentry, socket, etc.). The config hook now: - Registers missing MCP servers at plugin load time - Enforces eager/lazy loading policy (only osgrep eager) - Sets global tool permissions per MCP - Applies per-agent MCP tool enablement for subagents - Skips MCPs whose required binaries are not installed - Disables Oh-My-OpenCode tool patterns globally - Complements generate-opencode-agents.sh (shell script takes precedence) Design decision: chose data-driven registry over reading from mcp-servers-config.json.txt because the plugin needs runtime binary detection and platform-specific logic that a static config file cannot express.
…008.2) Documents the data-driven MCP registry, per-agent tool permissions, eager/lazy loading policy, and design decisions for Phase 2.
marcusquinn
force-pushed
the
feature/t008.2
branch
from
1a36dc7 to
0c41aa8
Compare
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Wed Feb 11 20:06:49 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
1 participant
Summary
MCP server registration for the aidevops OpenCode plugin (t008.2, Phase 2 of the aidevops-opencode plugin plan).
What's New
Registered MCPs
Design Decisions
Testing
Files Changed
Ref #1095
Summary by CodeRabbit
New Features
Documentation